U4-9406 - Create Scope object to wrap multiple Service calls to have a single transaction

Created by Shannon Deminick 19 Jan 2017, 03:07:51 Updated by Shannon Deminick 09 Feb 2017, 00:43:40

Relates to: U4-9477

Relates to: U4-9322

Subtask of: deploy-118

  • Create the Scope object
  • Update all database contexts, database, repository, services, units of work to work with the scope
  • Ensure backwards compatibility
  • Should not be a public API

Comments

Stephan 01 Feb 2017, 09:55:10

Part of U4-9322 branch - the Scope "framework" is not rather stable but I need to cleanup the code, remove stupid comments, etc. In Progress


Shannon Deminick 03 Feb 2017, 02:00:52

I'm going to mark this as a breaking change since there are some public APIs signatures that are changing - though these changes should not affect most installations since these APIs are considered internal and non-breaking: https://our.umbraco.org/Documentation/Development-Guidelines/breaking-changes. In theory these changes would only affect your unit tests.

  • Umbraco.Core.Persistence.RepositoryFactory signatures have changed
  • Umbraco.Core.Persistence.UmbracoDatabase is no longer IDisposeOnRequestEnd, IDisposable
  • FileUnitOfWorkProvider now inherits from ScopeUnitOfWorkProvider
  • All Umbraco.Core.Services.* inherit from ScopeRepositoryService
  • All IDatabaseUnitOfWorkProvider instances must actually implement IScopeUnitOfWorkProvider - internally this is the case

Some public APIs have changed that we actually cannot allow:

  • Umbraco.Core.DatabaseContext method signatures have changed and now people are no longer able to publicly create this for tests. So we either have to put back in the methods we've taken out to make them backwards compat or we have to document and prove (in our MockTests) that people can create or mock instances of this for use in their unit tests


Shannon Deminick 03 Feb 2017, 03:43:05

@zpqrtbnk I have found quite a few issues and I'm not sure how to solve it, I tried debugging scopes but can't really see what is going on since I don't really understand the whole AmbientScope thing. The problem has to do with the DefaultRepositoryCachePolicy.GetAll when GetAllCacheValidateCount == true and then it does a _options.PerformCount();, you will get this problem.

You can replicate by doing a few things:

  • Go edit a User

  • Expand User Type tree

  • Create a new document type

  • Add a property group

  • Add a property type

  • Add a name for the property type

  • Press the button to select an editor - this is when the problem occurs.

It won't happen every time, depends on if things are cached or not. DataTypeDefinitionRepository's CachePolicy which has it's option set to GetAllCacheValidateCount. Inside of DefaultRepositoryCachePolicy.GetAll it will call into var totalCount = _options.PerformCount(); and when it does this you'll get the exception that is thrown from the Scope throw new ObjectDisposedException("this");

If you put a breakpoint in the Scope.EnsureNotDisposed where it throws this exception you can see the stack trace. What I can't figure out is why this is happening, maybe this lookup needs it's own scope or something.

On another note which I've discovered by break pointing on the Scope.Dispose method, the old localization APIs: ui.Text are pretty nasty for perf because it will go lookup a User from the service everytime it's used, our Users are cached but it means that a scope is created for this everytime. Pretty sure in v8 I've already removed the whole ui class and this fix is there but for now, this should boost perf quite a lot! rev: cda9d0f9f26bded55f9d31665e873219214740a1

Cannot access a disposed object.
 Object name: 'this'.

System.ObjectDisposedException: Cannot access a disposed object.
 Object name: 'this'.

at Umbraco.Core.Scoping.Scope.EnsureNotDisposed() in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Core\Scoping\Scope.cs:line 254
   at Umbraco.Core.Scoping.Scope.get_Database() in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Core\Scoping\Scope.cs:line 148
   at Umbraco.Core.Persistence.UnitOfWork.ScopeUnitOfWork.get_Database() in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Core\Persistence\UnitOfWork\ScopeUnitOfWork.cs:line 162
   at Umbraco.Core.Persistence.Repositories.PetaPocoRepositoryBase`2.get_Database() in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Core\Persistence\Repositories\PetaPocoRepositoryBase.cs:line 39
   at Umbraco.Core.Persistence.Repositories.PetaPocoRepositoryBase`2.PerformCount(IQuery`1 query) in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Core\Persistence\Repositories\PetaPocoRepositoryBase.cs:line 67
   at Umbraco.Core.Persistence.Repositories.RepositoryBase`2.<get_DefaultOptions>b__9_0() in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Core\Persistence\Repositories\RepositoryBase.cs:line 139
   at Umbraco.Core.Cache.DefaultRepositoryCachePolicy`2.GetAll(TId[] ids, Func`2 performGetAll) in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Core\Cache\DefaultRepositoryCachePolicy.cs:line 213
   at Umbraco.Core.Persistence.Repositories.RepositoryBase`2.GetAll(TId[] ids) in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Core\Persistence\Repositories\RepositoryBase.cs:line 248
   at Umbraco.Core.Services.DataTypeService.GetAllDataTypeDefinitions(Int32[] ids) in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Core\Services\DataTypeService.cs:line 259
   at Umbraco.Web.Editors.DataTypeController.GetGroupedDataTypes() in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Web\Editors\DataTypeController.cs:line 300
   at lambda_method(Closure , Object , Object[] )
   at System.Web.Http.Controllers.ReflectedHttpActionDescriptor.ActionExecutor.<>c__DisplayClass10.<GetExecutor>b__9(Object instance, Object[] methodParameters)
   at System.Web.Http.Controllers.ReflectedHttpActionDescriptor.ExecuteAsync(HttpControllerContext controllerContext, IDictionary`2 arguments, CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Controllers.ApiControllerActionInvoker.<InvokeActionAsyncCore>d__0.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.ActionFilterAttribute.<ExecuteActionFilterAsyncCore>d__0.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.ActionFilterAttribute.<ExecuteActionFilterAsyncCore>d__0.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.ActionFilterAttribute.<ExecuteActionFilterAsyncCore>d__0.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Web.Http.Filters.ActionFilterAttribute.<CallOnActionExecutedAsync>d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.ActionFilterAttribute.<ExecuteActionFilterAsyncCore>d__0.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Controllers.ActionFilterResult.<ExecuteAsync>d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.AuthorizationFilterAttribute.<ExecuteAuthorizationFilterAsyncCore>d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.AuthorizationFilterAttribute.<ExecuteAuthorizationFilterAsyncCore>d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.AuthorizationFilterAttribute.<ExecuteAuthorizationFilterAsyncCore>d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.AuthorizationFilterAttribute.<ExecuteAuthorizationFilterAsyncCore>d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Dispatcher.HttpControllerDispatcher.<SendAsync>d__1.MoveNext()


Shannon Deminick 03 Feb 2017, 03:56:32

I've setup a test that shows this which may help you debug: CachedDataTypeServiceTests.DataTypeService_Can_Get_All


Shannon Deminick 03 Feb 2017, 04:03:50

More issues, I've just been navigating around and saving (not publishing content) and after a few I got this: (I do have DEBUG_SCOPES turned on)

System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
 Parameter name: index

at System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument, ExceptionResource resource)
   at System.Collections.Generic.List`1.RemoveAt(Int32 index)
   at System.Collections.Generic.List`1.Remove(T item)
   at Umbraco.Core.Scoping.ScopeProvider.Disposed(IScope scope)
   at Umbraco.Core.Scoping.Scope.Dispose()
   at Umbraco.Core.Persistence.UnitOfWork.ScopeUnitOfWork.DisposeResources()
   at Umbraco.Core.DisposableObject.Dispose(Boolean disposing)
   at Umbraco.Core.DisposableObject.Dispose()
   at Umbraco.Core.Services.UserService.GetUserById(Int32 id)
   at Umbraco.Web.Security.WebSecurity.get_CurrentUser() in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Web\Security\WebSecurity.cs:line 77
   at Umbraco.Web.WebApi.Filters.UmbracoApplicationAuthorizeAttribute.IsAuthorized(HttpActionContext actionContext) in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Web\WebApi\Filters\UmbracoApplicationAuthorizeAttribute.cs:line 37
   at System.Web.Http.AuthorizeAttribute.OnAuthorization(HttpActionContext actionContext)
   at Umbraco.Web.WebApi.Filters.OverridableAuthorizationAttribute.OnAuthorization(HttpActionContext actionContext) in X:\Projects\Umbraco\Umbraco_7.6\src\Umbraco.Web\WebApi\Filters\OverridableAuthorizationAttribute.cs:line 47
   at System.Web.Http.Filters.AuthorizationFilterAttribute.OnAuthorizationAsync(HttpActionContext actionContext, CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Filters.AuthorizationFilterAttribute.<ExecuteAuthorizationFilterAsyncCore>d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Web.Http.Dispatcher.HttpControllerDispatcher.<SendAsync>d__1.MoveNext()


Stephan 04 Feb 2017, 14:50:06

Have pushed 5a81a9eed7176047419ee6b413a13a5436ae2d8a that fixes this new test.


Shannon Deminick 08 Feb 2017, 11:32:34

@zpqrtbnk please see my latest commits, it fixes the issue of package uninstall and the strange sequence of events that will raise trashed events for content that is already deleted after the transaction is completed, this is done by making the content type deletion a bit smarter and more efficient to ensure that all content and content type descendants are done at once.


Shannon Deminick 08 Feb 2017, 11:34:06

Hrm, one thing to test though, does removing a document type that has content that has children of a different content type actually still send that child content to the trash? need to verify.


Shannon Deminick 08 Feb 2017, 23:37:17

Ok, I'm going to finally close this one, @zpqrtbnk you still need to review these though:

  • 351796fd6b80be544e27d7a20e088907c75bfda1
  • fb02a5b06de690a646a6f3c64f4551b8bbcc36ce
  • e06cda98e93522fe349f52bd2c459cbaf4e3109e
  • 4feb876463c2a7937e0bcd3130369c66be8bd416

These show the issue that when deleting a content type that has content that has children that have mixed types, some of the content type being deleted and some others, what used to happen was that all children would be moved to the bin, this would trigger Trashed events, then on the next iterations, some of those children would also be deleted because they were of this same doc type, then in an event handler like RelateOnTrashHandler, we would try to make a relation for a deleted item. This used to work because events would be raised before the deleted item was committed, but now the events are raised after the whole transaction is committed.

The logic in the DeleteContentOfType and DeleteMediaOfType (singular) is inefficient and causes this problem because during a content item's deletion, it will send all children to the trash and raise events for these children even if one of this childs is already flagged for deletion.

To fix this a few things have been done:

  • Internal logic separated into MoveToRecycleBinDo which allows passing in a set of ids to ignore for the children found because they are already flagged for deletion
  • More efficient logic for finding/iterating children - which used to find descendants of all children when we only need to find descendants of the root nodes of the current content set
  • New methods: DeleteContentOfTypes and DeleteMediaOfTypes which delete multiple content items of many types at the same time

I've then removed a ton of code duplication in rev: 2dc45c3b9fe8fe19cd7753c7ff06e0550f07671c - please read the commit message for that one.


Shannon Deminick 09 Feb 2017, 00:43:33

Ack, ok one more commit: 97e8973cbd1db5b49900eaf6abccd7609b047b90 even with all of the above fixes, there's still an issue with the RelateOnTrashHandler, it tries to create a relationship with the trashed item's original parent, however when you delete a content type that has content that has children of a different content type, those children are moved to the bin but the parent is deleted, so this relationship cannot be created, so we make sure to check it exists before continuing. Ok, now it's done.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty:

Category:

Backwards Compatible: False

Fix Submitted:

Affected versions:

Due in version: 7.6.0

Sprint: Sprint 52

Story Points: 5

Cycle: