From a2cc6a0a87f1500e8827b6afc49abd5c262f0d62 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Fri, 25 Jul 2025 13:07:20 +0200 Subject: [PATCH] Fix issue with use of EF Core scopes within notification handlers (take 2 - handling scopes with a base parent) (#19797) * Add integration tests that shows the problem * Fix the problem and add explenation * Improved comments slightly to help when we come back here! Moved tests alongside existing ones related to scopes. Removed long running attribute from tests (they are quite fast). * Fixed casing in comment. --------- Co-authored-by: Andy Butland --- .../Scoping/EFCoreScope.cs | 8 +- src/Umbraco.Core/Scoping/CoreScope.cs | 2 + .../Scoping/NestedScopeTests.cs | 222 ++++++++++++++++++ 3 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/NestedScopeTests.cs diff --git a/src/Umbraco.Cms.Persistence.EFCore/Scoping/EFCoreScope.cs b/src/Umbraco.Cms.Persistence.EFCore/Scoping/EFCoreScope.cs index 461b09334c..382bcf5593 100644 --- a/src/Umbraco.Cms.Persistence.EFCore/Scoping/EFCoreScope.cs +++ b/src/Umbraco.Cms.Persistence.EFCore/Scoping/EFCoreScope.cs @@ -127,10 +127,16 @@ internal class EFCoreScope : CoreScope, IEfCoreScope Locks.ClearLocks(InstanceId); - if (ParentScope is null) + // Since we can nest EFCoreScopes in other scopes derived from CoreScope, we should check whether our ParentScope OR the base ParentScope exists. + // Only if neither do do we take responsibility for ensuring the locks are cleared. + // Eventually the highest parent will clear the locks. + // Further, these locks are a reference to the locks of the highest parent anyway (see the constructor of CoreScope). +#pragma warning disable SA1100 // Do not prefix calls with base unless local implementation exists (justification: provides additional clarify here that this is defined on the base class). + if (ParentScope is null && base.HasParentScope is false) { Locks.EnsureLocksCleared(InstanceId); } +#pragma warning restore SA1100 // Do not prefix calls with base unless local implementation exists _efCoreScopeProvider.PopAmbientScope(); diff --git a/src/Umbraco.Core/Scoping/CoreScope.cs b/src/Umbraco.Core/Scoping/CoreScope.cs index 7fe6c400fb..e158641af4 100644 --- a/src/Umbraco.Core/Scoping/CoreScope.cs +++ b/src/Umbraco.Core/Scoping/CoreScope.cs @@ -250,6 +250,8 @@ public class CoreScope : ICoreScope _parentScope = coreScope; } + protected bool HasParentScope => _parentScope is not null; + protected void HandleScopedNotifications() => _notificationPublisher?.ScopeExit(Completed.HasValue && Completed.Value); private void EnsureNotDisposed() diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/NestedScopeTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/NestedScopeTests.cs new file mode 100644 index 0000000000..4d0587c236 --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/NestedScopeTests.cs @@ -0,0 +1,222 @@ +using Microsoft.Extensions.DependencyInjection; +using NUnit.Framework; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Scoping; +using Umbraco.Cms.Persistence.EFCore.Scoping; +using Umbraco.Cms.Tests.Common.Testing; +using Umbraco.Cms.Tests.Integration.Testing; +using Umbraco.Cms.Tests.Integration.Umbraco.Persistence.EFCore.DbContext; +using IScopeProvider = Umbraco.Cms.Infrastructure.Scoping.IScopeProvider; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping +{ + /// + /// These tests verify that the various types of scopes we have can be created and disposed within each other. + /// + /// + /// Scopes are: + /// - "Normal" - created by "/>. + /// - "Core" - created by "/>. + /// - "EFCore" - created by "/>. + /// + [TestFixture] + [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] + internal sealed class NestedScopeTests : UmbracoIntegrationTest + { + private new IScopeProvider ScopeProvider => Services.GetRequiredService(); + + private ICoreScopeProvider CoreScopeProvider => Services.GetRequiredService(); + + private IEFCoreScopeProvider EfCoreScopeProvider => + Services.GetRequiredService>(); + + [Test] + public void CanNestScopes_Normal_Core_EfCore() + { + using (var ambientScope = ScopeProvider.CreateScope()) + { + ambientScope.WriteLock(Constants.Locks.ContentTree); + + using (var outerScope = CoreScopeProvider.CreateCoreScope()) + { + outerScope.WriteLock(Constants.Locks.ContentTree); + + using (var innerScope = EfCoreScopeProvider.CreateScope()) + { + innerScope.WriteLock(Constants.Locks.ContentTree); + + innerScope.Complete(); + outerScope.Complete(); + ambientScope.Complete(); + } + } + } + } + + [Test] + public void CanNestScopes_Normal_EfCore_Core() + { + using (var ambientScope = ScopeProvider.CreateScope()) + { + ambientScope.WriteLock(Constants.Locks.ContentTree); + + using (var outerScope = EfCoreScopeProvider.CreateScope()) + { + outerScope.WriteLock(Constants.Locks.ContentTree); + + using (var innerScope = CoreScopeProvider.CreateCoreScope()) + { + innerScope.WriteLock(Constants.Locks.ContentTree); + + innerScope.Complete(); + outerScope.Complete(); + ambientScope.Complete(); + } + } + } + } + + [Test] + public void CanNestScopes_Core_Normal_Efcore() + { + using (var ambientScope = CoreScopeProvider.CreateCoreScope()) + { + ambientScope.WriteLock(Constants.Locks.ContentTree); + + using (var outerScope = ScopeProvider.CreateScope()) + { + outerScope.WriteLock(Constants.Locks.ContentTree); + + using (var innerScope = EfCoreScopeProvider.CreateScope()) + { + innerScope.WriteLock(Constants.Locks.ContentTree); + + innerScope.Complete(); + outerScope.Complete(); + ambientScope.Complete(); + } + } + } + } + + [Test] + public void CanNestScopes_Core_EfCore_Normal() + { + using (var ambientScope = CoreScopeProvider.CreateCoreScope()) + { + ambientScope.WriteLock(Constants.Locks.ContentTree); + + using (var outerScope = EfCoreScopeProvider.CreateScope()) + { + outerScope.WriteLock(Constants.Locks.ContentTree); + + using (var innerScope = ScopeProvider.CreateScope()) + { + innerScope.WriteLock(Constants.Locks.ContentTree); + + innerScope.Complete(); + outerScope.Complete(); + ambientScope.Complete(); + } + } + } + } + + [Test] + public void CanNestScopes_EfCore_Normal_Core() + { + using (var ambientScope = EfCoreScopeProvider.CreateScope()) + { + ambientScope.WriteLock(Constants.Locks.ContentTree); + + using (var outerScope = ScopeProvider.CreateScope()) + { + outerScope.WriteLock(Constants.Locks.ContentTree); + + using (var innerScope = CoreScopeProvider.CreateCoreScope()) + { + innerScope.WriteLock(Constants.Locks.ContentTree); + + innerScope.Complete(); + outerScope.Complete(); + ambientScope.Complete(); + } + } + } + } + + [Test] + public void CanNestScopes_EfCore_Core_Normal() + { + using (var ambientScope = EfCoreScopeProvider.CreateScope()) + { + ambientScope.WriteLock(Constants.Locks.ContentTree); + + using (var outerScope = CoreScopeProvider.CreateCoreScope()) + { + outerScope.WriteLock(Constants.Locks.ContentTree); + + using (var innerScope = ScopeProvider.CreateScope()) + { + innerScope.WriteLock(Constants.Locks.ContentTree); + + innerScope.Complete(); + outerScope.Complete(); + ambientScope.Complete(); + } + } + } + } + + [Test] + public void CanNestScopes_Normal_Normal() + { + using (var ambientScope = ScopeProvider.CreateScope()) + { + ambientScope.WriteLock(Constants.Locks.ContentTree); + + using (var inner = ScopeProvider.CreateScope()) + { + inner.WriteLock(Constants.Locks.ContentTree); + + inner.Complete(); + ambientScope.Complete(); + } + } + } + + [Test] + public void CanNestScopes_Core_Core() + { + using (var ambientScope = CoreScopeProvider.CreateCoreScope()) + { + ambientScope.WriteLock(Constants.Locks.ContentTree); + + using (var inner = CoreScopeProvider.CreateCoreScope()) + { + inner.WriteLock(Constants.Locks.ContentTree); + + inner.Complete(); + ambientScope.Complete(); + } + } + } + + [Test] + public void CanNestScopes_EfCore_EfCore() + { + using (var ambientScope = EfCoreScopeProvider.CreateScope()) + { + ambientScope.WriteLock(Constants.Locks.ContentTree); + + using (var inner = EfCoreScopeProvider.CreateScope()) + { + inner.WriteLock(Constants.Locks.ContentTree); + + inner.Complete(); + ambientScope.Complete(); + } + } + } + } +}