From 400b436d986d0924914e8bd3235f6497177c1b43 Mon Sep 17 00:00:00 2001 From: Mole Date: Mon, 1 Mar 2021 14:45:02 +0100 Subject: [PATCH 1/9] Fir stab/POC This still need ALOT of work, but is just an attempt to get my head around things. --- src/Umbraco.Core/Scoping/Scope.cs | 181 +++++++++++++++++++- src/Umbraco.Tests/Persistence/LocksTests.cs | 93 +++++++++- 2 files changed, 266 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 24ef92278cb6..efbfe2ded1a0 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Data; using Umbraco.Core.Cache; using Umbraco.Core.Composing; @@ -34,6 +35,15 @@ internal class Scope : IScope2 private ICompletable _fscope; private IEventDispatcher _eventDispatcher; + // ReadLocks and WriteLocks owned by the entire chain, managed by the outermost scope + public Dictionary ReadLocks; + public Dictionary WriteLocks; + + // ReadLocks and WriteLocks requested by this specific scope, required to be able to decrement + // Using HashSets works well assume that you're only gonna request a lock once pr. scope + private HashSet _readLocks; + private HashSet _writelocks; + // initializes a new scope private Scope(ScopeProvider scopeProvider, ILogger logger, FileSystems fileSystems, Scope parent, ScopeContext scopeContext, bool detachable, @@ -58,6 +68,11 @@ private Scope(ScopeProvider scopeProvider, Detachable = detachable; + ReadLocks = new Dictionary(); + WriteLocks = new Dictionary(); + _readLocks = new HashSet(); + _writelocks = new HashSet(); + #if DEBUG_SCOPES _scopeProvider.RegisterScope(this); Console.WriteLine("create " + InstanceId.ToString("N").Substring(0, 8)); @@ -348,6 +363,17 @@ public void Dispose() #endif } + // Decrement the lock counters + foreach (var readLockId in _readLocks) + { + DecrementReadLock(readLockId); + } + + foreach (var writeLockId in _writelocks) + { + DecrementWriteLock(writeLockId); + } + var parent = ParentScope; _scopeProvider.AmbientScope = parent; // might be null = this is how scopes are removed from context objects @@ -486,32 +512,179 @@ private static void TryFinally(int index, Action[] actions) private static bool LogUncompletedScopes => (_logUncompletedScopes ?? (_logUncompletedScopes = Current.Configs.CoreDebug().LogUncompletedScopes)).Value; + /// + /// Decrements the count of the ReadLocks with a specific ID we currently hold + /// + /// Lock ID to decrement + public void DecrementReadLock(int lockId) + { + if (ParentScope != null) + { + ParentScope.DecrementReadLock(lockId); + } + else + { + ReadLocks[lockId] -= 1; + } + } + + /// + /// Decrements the count of the WriteLocks with a specific ID we currently hold + /// + /// Lock ID to decrement + public void DecrementWriteLock(int lockId) + { + if (ParentScope != null) + { + ParentScope.DecrementWriteLock(lockId); + } + else + { + WriteLocks[lockId] -= 1; + } + } + /// - public void ReadLock(params int[] lockIds) => Database.SqlContext.SqlSyntax.ReadLock(Database, lockIds); + public void ReadLock(params int[] lockIds) + { + if (ParentScope != null) + { + foreach (var id in lockIds) + { + // We need to keep track of what lockIds we have requests to be able to decrement them. + // We have to do this out of the recursion to not add the ids to all scopes. + _readLocks.Add(id); + } + } + // Delegate acquiring the lock to the parent. + ReadLockInner(lockIds); + } + + internal void ReadLockInner(params int[] lockIds) + { + if (ParentScope != null) + { + // Delegate acquiring the lock to the parent. + ParentScope.ReadLockInner(lockIds); + return; + } + + foreach (var lockId in lockIds) + { + // Only acquire the lock if we haven't done so yet. + if (!ReadLocks.ContainsKey(lockId)) + { + Database.SqlContext.SqlSyntax.ReadLock(Database, lockId); + ReadLocks[lockId] = 0; + } + + // Increment the amount of locks we have of the specific ID after we've acquired it. + ReadLocks[lockId] += 1; + } + } /// public void ReadLock(TimeSpan timeout, int lockId) { + if (ParentScope != null) + { + _readLocks.Add(lockId); + } + + ReadLockInner(timeout, lockId); + } + + internal void ReadLockInner(TimeSpan timeout, int lockId) + { + if (ParentScope != null) + { + ParentScope.ReadLockInner(timeout, lockId); + return; + } + var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; if (syntax2 == null) { throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); } - syntax2.ReadLock(Database, timeout, lockId); + + if (!ReadLocks.ContainsKey(lockId)) + { + syntax2.ReadLock(Database, timeout, lockId); + ReadLocks[lockId] = 0; + } + + ReadLocks[lockId] += 1; } /// - public void WriteLock(params int[] lockIds) => Database.SqlContext.SqlSyntax.WriteLock(Database, lockIds); + public void WriteLock(params int[] lockIds) + { + // For some reason adding ids to the writelocks set of the outer most parent scope causes issues... + if (ParentScope != null) + { + foreach (var lockId in lockIds) + { + _writelocks.Add(lockId); + } + } + WriteLockInner(lockIds); + } + + internal void WriteLockInner(int[] lockIds) + { + // If we have a parent we just delegate lock creation to parent. + if (ParentScope != null) + { + ParentScope.WriteLockInner(lockIds); + return; + } + + // Only acquire lock if we haven't yet (WriteLocks not containing the key) + foreach (var lockId in lockIds) + { + if (!WriteLocks.ContainsKey(lockId)) + { + Database.SqlContext.SqlSyntax.WriteLock(Database, lockId); + WriteLocks[lockId] = 0; + } + + // Increment count of the lock by 1. + WriteLocks[lockId] += 1; + } + } /// public void WriteLock(TimeSpan timeout, int lockId) { + if (ParentScope != null) + { + _writelocks.Add(lockId); + } + WriteLockInner(timeout, lockId); + } + + internal void WriteLockInner(TimeSpan timeout, int lockId) + { + if (ParentScope != null) + { + ParentScope.WriteLockInner(timeout, lockId); + return; + } + var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; if (syntax2 == null) { throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); } - syntax2.WriteLock(Database, timeout, lockId); + + if (!WriteLocks.ContainsKey(lockId)) + { + syntax2.WriteLock(Database, timeout, lockId); + WriteLocks[lockId] = 0; + } + + WriteLocks[lockId] += 1; } } } diff --git a/src/Umbraco.Tests/Persistence/LocksTests.cs b/src/Umbraco.Tests/Persistence/LocksTests.cs index 1c651b9040ac..27a2947dbec2 100644 --- a/src/Umbraco.Tests/Persistence/LocksTests.cs +++ b/src/Umbraco.Tests/Persistence/LocksTests.cs @@ -280,7 +280,7 @@ public void NoDeadLockTest() [Test] public void Throws_When_Lock_Timeout_Is_Exceeded() - { + { var t1 = Task.Run(() => { using (var scope = ScopeProvider.CreateScope()) @@ -288,7 +288,7 @@ public void Throws_When_Lock_Timeout_Is_Exceeded() var realScope = (Scope)scope; Console.WriteLine("Write lock A"); - // This will acquire right away + // This will acquire right away realScope.WriteLock(TimeSpan.FromMilliseconds(2000), Constants.Locks.ContentTree); Thread.Sleep(6000); // Wait longer than the Read Lock B timeout scope.Complete(); @@ -349,7 +349,7 @@ public void Read_Lock_Waits_For_Write_Lock() var realScope = (Scope)scope; Console.WriteLine("Write lock A"); - // This will acquire right away + // This will acquire right away realScope.WriteLock(TimeSpan.FromMilliseconds(2000), Constants.Locks.ContentTree); Thread.Sleep(4000); // Wait less than the Read Lock B timeout scope.Complete(); @@ -377,7 +377,7 @@ public void Read_Lock_Waits_For_Write_Lock() scope.Complete(); Interlocked.Increment(ref locksCompleted); Console.WriteLine("Finished Read lock B"); - } + } }); var t3 = Task.Run(() => @@ -424,6 +424,91 @@ public void Lock_Exceeds_Command_Timeout() } } + [Test] + public void Nested_Scopes_WriteLocks_Count_Correctly() + { + using (var scope = ScopeProvider.CreateScope()) + { + var parentScope = (Scope) scope; + scope.WriteLock(Constants.Locks.ContentTree); + scope.WriteLock(Constants.Locks.ContentTypes); + + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTree], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + + using (var childScope1 = ScopeProvider.CreateScope()) + { + childScope1.WriteLock(Constants.Locks.ContentTree); + childScope1.WriteLock(Constants.Locks.ContentTypes); + childScope1.WriteLock(Constants.Locks.Languages); + + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTree], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.Languages], $"childScope1 after locks acquired: {nameof(Constants.Locks.Languages)}"); + + using (var childScope2 = ScopeProvider.CreateScope()) + { + childScope2.WriteLock(Constants.Locks.ContentTree); + childScope2.WriteLock(Constants.Locks.MediaTypes); + + Assert.AreEqual(3, parentScope.WriteLocks[Constants.Locks.ContentTree], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.Languages], $"childScope2 after locks acquired: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.MediaTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.MediaTypes)}"); + } + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTree], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.Languages], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(0, parentScope.WriteLocks[Constants.Locks.MediaTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.MediaTypes)}"); + } + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(0, parentScope.WriteLocks[Constants.Locks.Languages], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(0, parentScope.WriteLocks[Constants.Locks.MediaTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.MediaTypes)}"); + } + } + + [Test] + public void Nested_Scopes_ReadLocks_Count_Correctly() + { + using (var scope = ScopeProvider.CreateScope()) + { + var parentScope = (Scope) scope; + scope.ReadLock(Constants.Locks.ContentTree); + scope.ReadLock(Constants.Locks.ContentTypes); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTree], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + + using (var childScope1 = ScopeProvider.CreateScope()) + { + childScope1.ReadLock(Constants.Locks.ContentTree); + childScope1.ReadLock(Constants.Locks.ContentTypes); + childScope1.ReadLock(Constants.Locks.Languages); + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTree], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.Languages], $"childScope1 after locks acquired: {nameof(Constants.Locks.Languages)}"); + + using (var childScope2 = ScopeProvider.CreateScope()) + { + childScope2.ReadLock(Constants.Locks.ContentTree); + childScope2.ReadLock(Constants.Locks.MediaTypes); + Assert.AreEqual(3, parentScope.ReadLocks[Constants.Locks.ContentTree], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.Languages], $"childScope2 after locks acquired: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.MediaTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.MediaTypes)}"); + } + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTree], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.Languages], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(0, parentScope.ReadLocks[Constants.Locks.MediaTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.MediaTypes)}"); + } + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(0, parentScope.ReadLocks[Constants.Locks.Languages], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(0, parentScope.ReadLocks[Constants.Locks.MediaTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.MediaTypes)}"); + } + } + private void NoDeadLockTestThread(int id, EventWaitHandle myEv, WaitHandle otherEv, ref Exception exception) { using (var scope = ScopeProvider.CreateScope()) From 65f2c5ee705d24964db3267af91aefda4f3cba37 Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 2 Mar 2021 08:48:09 +0100 Subject: [PATCH 2/9] Bit of cleaning in Scope --- src/Umbraco.Core/Scoping/Scope.cs | 48 +++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index efbfe2ded1a0..e79fc4ad1945 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -40,9 +40,9 @@ internal class Scope : IScope2 public Dictionary WriteLocks; // ReadLocks and WriteLocks requested by this specific scope, required to be able to decrement - // Using HashSets works well assume that you're only gonna request a lock once pr. scope - private HashSet _readLocks; - private HashSet _writelocks; + // Using Lists just in case someone for some reason requests the same lock twice in the same scope. + private List _readLocks; + private List _writelocks; // initializes a new scope private Scope(ScopeProvider scopeProvider, @@ -70,8 +70,8 @@ private Scope(ScopeProvider scopeProvider, ReadLocks = new Dictionary(); WriteLocks = new Dictionary(); - _readLocks = new HashSet(); - _writelocks = new HashSet(); + _readLocks = new List(); + _writelocks = new List(); #if DEBUG_SCOPES _scopeProvider.RegisterScope(this); @@ -518,14 +518,14 @@ private static void TryFinally(int index, Action[] actions) /// Lock ID to decrement public void DecrementReadLock(int lockId) { + // If we aren't the outermost scope, pass it on to the parent. if (ParentScope != null) { ParentScope.DecrementReadLock(lockId); + return; } - else - { - ReadLocks[lockId] -= 1; - } + + ReadLocks[lockId] -= 1; } /// @@ -534,14 +534,14 @@ public void DecrementReadLock(int lockId) /// Lock ID to decrement public void DecrementWriteLock(int lockId) { + // If we aren't the outermost scope, pass it on to the parent. if (ParentScope != null) { ParentScope.DecrementWriteLock(lockId); + return; } - else - { - WriteLocks[lockId] -= 1; - } + + WriteLocks[lockId] -= 1; } /// @@ -551,15 +551,18 @@ public void ReadLock(params int[] lockIds) { foreach (var id in lockIds) { - // We need to keep track of what lockIds we have requests to be able to decrement them. + // We need to keep track of what lockIds we have requested locks for to be able to decrement them. // We have to do this out of the recursion to not add the ids to all scopes. _readLocks.Add(id); } } - // Delegate acquiring the lock to the parent. ReadLockInner(lockIds); } + /// + /// Handles acquiring read locks, will delegate it to the parent if there are any. + /// + /// Array of lock object identifiers. internal void ReadLockInner(params int[] lockIds) { if (ParentScope != null) @@ -569,6 +572,7 @@ internal void ReadLockInner(params int[] lockIds) return; } + // If we are the parent, then handle the lock request. foreach (var lockId in lockIds) { // Only acquire the lock if we haven't done so yet. @@ -594,6 +598,11 @@ public void ReadLock(TimeSpan timeout, int lockId) ReadLockInner(timeout, lockId); } + /// + /// Handles acquiring a read lock with a specified timeout, will delegate it to the parent if there are any. + /// + /// The database timeout in milliseconds. + /// The lock object identifier. internal void ReadLockInner(TimeSpan timeout, int lockId) { if (ParentScope != null) @@ -631,6 +640,10 @@ public void WriteLock(params int[] lockIds) WriteLockInner(lockIds); } + /// + /// Handles acquiring write locks, will delegate it to the parent if there are any. + /// + /// Array of lock object identifiers. internal void WriteLockInner(int[] lockIds) { // If we have a parent we just delegate lock creation to parent. @@ -664,6 +677,11 @@ public void WriteLock(TimeSpan timeout, int lockId) WriteLockInner(timeout, lockId); } + /// + /// Handles acquiring a write lock with a specified timeout, will delegate it to the parent if there are any. + /// + /// The database timeout in milliseconds. + /// The lock object identifier. internal void WriteLockInner(TimeSpan timeout, int lockId) { if (ParentScope != null) From e75edbcaf0469b7696629817a796a2eed61e2c42 Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 2 Mar 2021 10:53:10 +0100 Subject: [PATCH 3/9] Add more tests. --- src/Umbraco.Tests/Persistence/LocksTests.cs | 86 ------ src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 280 ++++++++++++++++++++ src/Umbraco.Tests/Umbraco.Tests.csproj | 1 + 3 files changed, 281 insertions(+), 86 deletions(-) create mode 100644 src/Umbraco.Tests/Scoping/ScopeUnitTests.cs diff --git a/src/Umbraco.Tests/Persistence/LocksTests.cs b/src/Umbraco.Tests/Persistence/LocksTests.cs index 27a2947dbec2..88723292843b 100644 --- a/src/Umbraco.Tests/Persistence/LocksTests.cs +++ b/src/Umbraco.Tests/Persistence/LocksTests.cs @@ -1,5 +1,4 @@ using System; -using System.Collections.Generic; using System.Data.SqlServerCe; using System.Linq; using System.Threading; @@ -424,91 +423,6 @@ public void Lock_Exceeds_Command_Timeout() } } - [Test] - public void Nested_Scopes_WriteLocks_Count_Correctly() - { - using (var scope = ScopeProvider.CreateScope()) - { - var parentScope = (Scope) scope; - scope.WriteLock(Constants.Locks.ContentTree); - scope.WriteLock(Constants.Locks.ContentTypes); - - Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTree], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); - - using (var childScope1 = ScopeProvider.CreateScope()) - { - childScope1.WriteLock(Constants.Locks.ContentTree); - childScope1.WriteLock(Constants.Locks.ContentTypes); - childScope1.WriteLock(Constants.Locks.Languages); - - Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTree], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.Languages], $"childScope1 after locks acquired: {nameof(Constants.Locks.Languages)}"); - - using (var childScope2 = ScopeProvider.CreateScope()) - { - childScope2.WriteLock(Constants.Locks.ContentTree); - childScope2.WriteLock(Constants.Locks.MediaTypes); - - Assert.AreEqual(3, parentScope.WriteLocks[Constants.Locks.ContentTree], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.Languages], $"childScope2 after locks acquired: {nameof(Constants.Locks.Languages)}"); - Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.MediaTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.MediaTypes)}"); - } - Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTree], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.Languages], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.Languages)}"); - Assert.AreEqual(0, parentScope.WriteLocks[Constants.Locks.MediaTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.MediaTypes)}"); - } - Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(0, parentScope.WriteLocks[Constants.Locks.Languages], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.Languages)}"); - Assert.AreEqual(0, parentScope.WriteLocks[Constants.Locks.MediaTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.MediaTypes)}"); - } - } - - [Test] - public void Nested_Scopes_ReadLocks_Count_Correctly() - { - using (var scope = ScopeProvider.CreateScope()) - { - var parentScope = (Scope) scope; - scope.ReadLock(Constants.Locks.ContentTree); - scope.ReadLock(Constants.Locks.ContentTypes); - Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTree], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); - - using (var childScope1 = ScopeProvider.CreateScope()) - { - childScope1.ReadLock(Constants.Locks.ContentTree); - childScope1.ReadLock(Constants.Locks.ContentTypes); - childScope1.ReadLock(Constants.Locks.Languages); - Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTree], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.Languages], $"childScope1 after locks acquired: {nameof(Constants.Locks.Languages)}"); - - using (var childScope2 = ScopeProvider.CreateScope()) - { - childScope2.ReadLock(Constants.Locks.ContentTree); - childScope2.ReadLock(Constants.Locks.MediaTypes); - Assert.AreEqual(3, parentScope.ReadLocks[Constants.Locks.ContentTree], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.Languages], $"childScope2 after locks acquired: {nameof(Constants.Locks.Languages)}"); - Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.MediaTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.MediaTypes)}"); - } - Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTree], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.Languages], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.Languages)}"); - Assert.AreEqual(0, parentScope.ReadLocks[Constants.Locks.MediaTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.MediaTypes)}"); - } - Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(0, parentScope.ReadLocks[Constants.Locks.Languages], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.Languages)}"); - Assert.AreEqual(0, parentScope.ReadLocks[Constants.Locks.MediaTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.MediaTypes)}"); - } - } - private void NoDeadLockTestThread(int id, EventWaitHandle myEv, WaitHandle otherEv, ref Exception exception) { using (var scope = ScopeProvider.CreateScope()) diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs new file mode 100644 index 000000000000..34d1843dc465 --- /dev/null +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -0,0 +1,280 @@ +using System; +using Moq; +using NPoco; +using NUnit.Framework; +using Umbraco.Core; +using Umbraco.Core.Composing; +using Umbraco.Core.IO; +using Umbraco.Core.Logging; +using Umbraco.Core.Persistence; +using Umbraco.Core.Persistence.SqlSyntax; +using Umbraco.Core.Scoping; + +namespace Umbraco.Tests.Scoping +{ + [TestFixture] + public class ScopeUnitTests + { + /// + /// Creates a ScopeProvider with mocked internals. + /// + /// The mock of the ISqlSyntaxProvider2, used to count method calls. + /// + private ScopeProvider GetScopeProvider(out Mock syntaxProviderMock) + { + var logger = Mock.Of(); + var fac = Mock.Of(); + var fileSystem = new FileSystems(fac, logger); + var databaseFactory = new Mock(); + var database = new Mock(); + var sqlContext = new Mock(); + syntaxProviderMock = new Mock(); + + // Setup mock of database factory to return mock of database. + databaseFactory.Setup(x => x.CreateDatabase()).Returns(database.Object); + + // Setup mock of database to return mock of sql SqlContext + database.Setup(x => x.SqlContext).Returns(sqlContext.Object); + + // Setup mock of ISqlContext to return syntaxProviderMock + sqlContext.Setup(x => x.SqlSyntax).Returns(syntaxProviderMock.Object); + + return new ScopeProvider(databaseFactory.Object, fileSystem, logger); + } + + [Test] + public void WriteLock_Acquired_Only_Once_Per_Key() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var outerScope = scopeProvider.CreateScope()) + { + outerScope.WriteLock(Constants.Locks.Domains); + outerScope.WriteLock(Constants.Locks.Languages); + + using (var innerScope1 = scopeProvider.CreateScope()) + { + innerScope1.WriteLock(Constants.Locks.Domains); + innerScope1.WriteLock(Constants.Locks.Languages); + + using (var innerScope2 = scopeProvider.CreateScope()) + { + innerScope2.WriteLock(Constants.Locks.Domains); + innerScope2.WriteLock(Constants.Locks.Languages); + innerScope2.Complete(); + } + innerScope1.Complete(); + } + outerScope.Complete(); + } + + syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.Domains), Times.Once); + syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.Languages), Times.Once); + } + + [Test] + public void WriteLock_With_Timeout_Acquired_Only_Once_Per_Key(){ + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + var timeout = TimeSpan.FromMilliseconds(10000); + + using (var outerScope = scopeProvider.CreateScope()) + { + var realScope = (Scope) outerScope; + realScope.WriteLock(timeout, Constants.Locks.Domains); + realScope.WriteLock(timeout, Constants.Locks.Languages); + + using (var innerScope1 = scopeProvider.CreateScope()) + { + var realInnerScope1 = (Scope) outerScope; + realInnerScope1.WriteLock(timeout, Constants.Locks.Domains); + realInnerScope1.WriteLock(timeout, Constants.Locks.Languages); + + using (var innerScope2 = scopeProvider.CreateScope()) + { + var realInnerScope2 = (Scope) innerScope2; + realInnerScope2.WriteLock(timeout, Constants.Locks.Domains); + realInnerScope2.WriteLock(timeout, Constants.Locks.Languages); + innerScope2.Complete(); + } + innerScope1.Complete(); + } + + outerScope.Complete(); + } + + syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), timeout, Constants.Locks.Domains), Times.Once); + syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), timeout, Constants.Locks.Languages), Times.Once); + } + + [Test] + public void ReadLock_Acquired_Only_Once_Per_Key() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var outerScope = scopeProvider.CreateScope()) + { + outerScope.ReadLock(Constants.Locks.Domains); + outerScope.ReadLock(Constants.Locks.Languages); + + using (var innerScope1 = scopeProvider.CreateScope()) + { + innerScope1.ReadLock(Constants.Locks.Domains); + innerScope1.ReadLock(Constants.Locks.Languages); + + using (var innerScope2 = scopeProvider.CreateScope()) + { + innerScope2.ReadLock(Constants.Locks.Domains); + innerScope2.ReadLock(Constants.Locks.Languages); + + innerScope2.Complete(); + } + + innerScope1.Complete(); + } + + outerScope.Complete(); + } + + syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), Constants.Locks.Domains), Times.Once); + syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), Constants.Locks.Languages), Times.Once); + } + + [Test] + public void ReadLock_With_Timeout_Acquired_Only_Once_Per_Key() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + var timeOut = TimeSpan.FromMilliseconds(10000); + + using (var outerScope = scopeProvider.CreateScope()) + { + var realOuterScope = (Scope) outerScope; + realOuterScope.ReadLock(timeOut, Constants.Locks.Domains); + realOuterScope.ReadLock(timeOut, Constants.Locks.Languages); + + using (var innerScope1 = scopeProvider.CreateScope()) + { + var realInnerScope1 = (Scope) innerScope1; + realInnerScope1.ReadLock(timeOut, Constants.Locks.Domains); + realInnerScope1.ReadLock(timeOut, Constants.Locks.Languages); + + using (var innerScope2 = scopeProvider.CreateScope()) + { + var realInnerScope2 = (Scope) innerScope2; + realInnerScope2.ReadLock(timeOut, Constants.Locks.Domains); + realInnerScope2.ReadLock(timeOut, Constants.Locks.Languages); + + innerScope2.Complete(); + } + + innerScope1.Complete(); + } + + outerScope.Complete(); + } + + syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), timeOut, Constants.Locks.Domains), Times.Once); + syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), timeOut, Constants.Locks.Languages), Times.Once); + } + + [Test] + public void Nested_Scopes_WriteLocks_Count_Correctly() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var outerScope = scopeProvider.CreateScope()) + { + var parentScope = (Scope) outerScope; + outerScope.WriteLock(Constants.Locks.ContentTree); + outerScope.WriteLock(Constants.Locks.ContentTypes); + + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTree], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + + using (var innerScope1 = scopeProvider.CreateScope()) + { + innerScope1.WriteLock(Constants.Locks.ContentTree); + innerScope1.WriteLock(Constants.Locks.ContentTypes); + innerScope1.WriteLock(Constants.Locks.Languages); + + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTree], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.Languages], $"childScope1 after locks acquired: {nameof(Constants.Locks.Languages)}"); + + using (var innerScope2 = scopeProvider.CreateScope()) + { + innerScope2.WriteLock(Constants.Locks.ContentTree); + innerScope2.WriteLock(Constants.Locks.MediaTypes); + + Assert.AreEqual(3, parentScope.WriteLocks[Constants.Locks.ContentTree], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.Languages], $"childScope2 after locks acquired: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.MediaTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.MediaTypes)}"); + + innerScope2.Complete(); + } + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTree], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.Languages], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(0, parentScope.WriteLocks[Constants.Locks.MediaTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.MediaTypes)}"); + + innerScope1.Complete(); + } + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, parentScope.WriteLocks[Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(0, parentScope.WriteLocks[Constants.Locks.Languages], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(0, parentScope.WriteLocks[Constants.Locks.MediaTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.MediaTypes)}"); + + outerScope.Complete(); + } + } + + [Test] + public void Nested_Scopes_ReadLocks_Count_Correctly() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var outerScope = scopeProvider.CreateScope()) + { + var parentScope = (Scope) outerScope; + outerScope.ReadLock(Constants.Locks.ContentTree); + outerScope.ReadLock(Constants.Locks.ContentTypes); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTree], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + + using (var innserScope1 = scopeProvider.CreateScope()) + { + innserScope1.ReadLock(Constants.Locks.ContentTree); + innserScope1.ReadLock(Constants.Locks.ContentTypes); + innserScope1.ReadLock(Constants.Locks.Languages); + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTree], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"childScope1 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.Languages], $"childScope1 after locks acquired: {nameof(Constants.Locks.Languages)}"); + + using (var innerScope2 = scopeProvider.CreateScope()) + { + innerScope2.ReadLock(Constants.Locks.ContentTree); + innerScope2.ReadLock(Constants.Locks.MediaTypes); + Assert.AreEqual(3, parentScope.ReadLocks[Constants.Locks.ContentTree], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.Languages], $"childScope2 after locks acquired: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.MediaTypes], $"childScope2 after locks acquired: {nameof(Constants.Locks.MediaTypes)}"); + + innerScope2.Complete(); + } + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTree], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(2, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.Languages], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(0, parentScope.ReadLocks[Constants.Locks.MediaTypes], $"childScope1 after inner scope disposed: {nameof(Constants.Locks.MediaTypes)}"); + + innserScope1.Complete(); + } + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, parentScope.ReadLocks[Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(0, parentScope.ReadLocks[Constants.Locks.Languages], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(0, parentScope.ReadLocks[Constants.Locks.MediaTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.MediaTypes)}"); + + outerScope.Complete(); + } + } + } +} diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index 97604df0c6d2..3059119dd498 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -161,6 +161,7 @@ + From 8c99a8e8a7952d80603515ba24f31fe9b8410815 Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 2 Mar 2021 12:55:18 +0100 Subject: [PATCH 4/9] Combine the two lock inner methods into one And add a couple of more unit tests --- src/Umbraco.Core/Scoping/Scope.cs | 174 +++++++++++--------- src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 58 +++++++ 2 files changed, 151 insertions(+), 81 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index e79fc4ad1945..f04d827209be 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -513,9 +513,9 @@ private static void TryFinally(int index, Action[] actions) ?? (_logUncompletedScopes = Current.Configs.CoreDebug().LogUncompletedScopes)).Value; /// - /// Decrements the count of the ReadLocks with a specific ID we currently hold + /// Decrements the count of the ReadLocks with a specific lock object identifier we currently hold /// - /// Lock ID to decrement + /// Lock object identifier to decrement public void DecrementReadLock(int lockId) { // If we aren't the outermost scope, pass it on to the parent. @@ -529,9 +529,9 @@ public void DecrementReadLock(int lockId) } /// - /// Decrements the count of the WriteLocks with a specific ID we currently hold + /// Decrements the count of the WriteLocks with a specific lock object identifier we currently hold. /// - /// Lock ID to decrement + /// Lock object identifier to decrement. public void DecrementWriteLock(int lockId) { // If we aren't the outermost scope, pass it on to the parent. @@ -556,109 +556,109 @@ public void ReadLock(params int[] lockIds) _readLocks.Add(id); } } - ReadLockInner(lockIds); + ReadLockInner(null, lockIds); } - /// - /// Handles acquiring read locks, will delegate it to the parent if there are any. - /// - /// Array of lock object identifiers. - internal void ReadLockInner(params int[] lockIds) + /// + public void ReadLock(TimeSpan timeout, int lockId) { if (ParentScope != null) { - // Delegate acquiring the lock to the parent. - ParentScope.ReadLockInner(lockIds); - return; + _readLocks.Add(lockId); } - // If we are the parent, then handle the lock request. - foreach (var lockId in lockIds) + ReadLockInner(timeout, lockId); + } + + /// + public void WriteLock(params int[] lockIds) + { + if (ParentScope != null) { - // Only acquire the lock if we haven't done so yet. - if (!ReadLocks.ContainsKey(lockId)) + foreach (var lockId in lockIds) { - Database.SqlContext.SqlSyntax.ReadLock(Database, lockId); - ReadLocks[lockId] = 0; + _writelocks.Add(lockId); } - - // Increment the amount of locks we have of the specific ID after we've acquired it. - ReadLocks[lockId] += 1; } + WriteLockInner(null, lockIds); } /// - public void ReadLock(TimeSpan timeout, int lockId) + public void WriteLock(TimeSpan timeout, int lockId) { if (ParentScope != null) { - _readLocks.Add(lockId); + _writelocks.Add(lockId); } - - ReadLockInner(timeout, lockId); + WriteLockInner(timeout, lockId); } /// - /// Handles acquiring a read lock with a specified timeout, will delegate it to the parent if there are any. + /// Handles acquiring a read lock, will delegate it to the parent if there are any. /// - /// The database timeout in milliseconds. - /// The lock object identifier. - internal void ReadLockInner(TimeSpan timeout, int lockId) + /// Optional database timeout in milliseconds. + /// Array of lock object identifiers. + internal void ReadLockInner(TimeSpan? timeout = null, params int[] lockIds) { if (ParentScope != null) { - ParentScope.ReadLockInner(timeout, lockId); + // Delegate acquiring the lock to the parent if any. + ParentScope.ReadLockInner(timeout, lockIds); return; } - var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; - if (syntax2 == null) - { - throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); - } - - if (!ReadLocks.ContainsKey(lockId)) - { - syntax2.ReadLock(Database, timeout, lockId); - ReadLocks[lockId] = 0; - } - - ReadLocks[lockId] += 1; - } - - /// - public void WriteLock(params int[] lockIds) - { - // For some reason adding ids to the writelocks set of the outer most parent scope causes issues... - if (ParentScope != null) + // If we are the parent, then handle the lock request. + foreach (var lockId in lockIds) { - foreach (var lockId in lockIds) + // Only acquire the lock if we haven't done so yet. + if (!ReadLocks.ContainsKey(lockId)) { - _writelocks.Add(lockId); + if (timeout is null) + { + // We want a lock with a custom timeout + ObtainReadLock(lockId); + } + else + { + // We just want an ordinary lock. + ObtainTimoutReadLock(lockId, timeout.Value); + } + // Add the lockId as a key to the dict. + ReadLocks[lockId] = 0; } + + ReadLocks[lockId] += 1; } - WriteLockInner(lockIds); } /// - /// Handles acquiring write locks, will delegate it to the parent if there are any. + /// Handles acquiring a write lock with a specified timeout, will delegate it to the parent if there are any. /// + /// Optional database timeout in milliseconds. /// Array of lock object identifiers. - internal void WriteLockInner(int[] lockIds) + internal void WriteLockInner(TimeSpan? timeout = null, params int[] lockIds) { - // If we have a parent we just delegate lock creation to parent. if (ParentScope != null) { - ParentScope.WriteLockInner(lockIds); + // If we have a parent we delegate lock creation to parent. + ParentScope.WriteLockInner(timeout, lockIds); return; } - // Only acquire lock if we haven't yet (WriteLocks not containing the key) foreach (var lockId in lockIds) { + // Only acquire lock if we haven't yet (WriteLocks not containing the key) if (!WriteLocks.ContainsKey(lockId)) { - Database.SqlContext.SqlSyntax.WriteLock(Database, lockId); + if (timeout is null) + { + ObtainWriteLock(lockId); + } + else + { + ObtainTimeoutWriteLock(lockId, timeout.Value); + } + // Add the lockId as a key to the dict. WriteLocks[lockId] = 0; } @@ -667,42 +667,54 @@ internal void WriteLockInner(int[] lockIds) } } - /// - public void WriteLock(TimeSpan timeout, int lockId) + /// + /// Obtains an ordinary read lock. + /// + /// Lock object identifier to lock. + private void ObtainReadLock(int lockId) { - if (ParentScope != null) - { - _writelocks.Add(lockId); - } - WriteLockInner(timeout, lockId); + Database.SqlContext.SqlSyntax.ReadLock(Database, lockId); } /// - /// Handles acquiring a write lock with a specified timeout, will delegate it to the parent if there are any. + /// Obtains a read lock with a custom timeout. /// - /// The database timeout in milliseconds. - /// The lock object identifier. - internal void WriteLockInner(TimeSpan timeout, int lockId) + /// Lock object identifier to lock. + /// TimeSpan specifying the timout period. + private void ObtainTimoutReadLock(int lockId, TimeSpan timeout) { - if (ParentScope != null) - { - ParentScope.WriteLockInner(timeout, lockId); - return; - } - var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; if (syntax2 == null) { throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); } - if (!WriteLocks.ContainsKey(lockId)) + syntax2.ReadLock(Database, timeout, lockId); + } + + /// + /// Obtains an ordinary write lock. + /// + /// Lock object identifier to lock. + private void ObtainWriteLock(int lockId) + { + Database.SqlContext.SqlSyntax.WriteLock(Database, lockId); + } + + /// + /// Obtains a write lock with a custom timeout. + /// + /// Lock object identifier to lock. + /// TimeSpan specifying the timout period. + private void ObtainTimeoutWriteLock(int lockId, TimeSpan timeout) + { + var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; + if (syntax2 == null) { - syntax2.WriteLock(Database, timeout, lockId); - WriteLocks[lockId] = 0; + throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); } - WriteLocks[lockId] += 1; + syntax2.WriteLock(Database, timeout, lockId); } } } diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index 34d1843dc465..32bd7e2afe5a 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -176,6 +176,64 @@ public void ReadLock_With_Timeout_Acquired_Only_Once_Per_Key() syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), timeOut, Constants.Locks.Languages), Times.Once); } + [Test] + public void WriteLocks_Count_correctly_If_Lock_Requested_Twice_In_Scope() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var outerscope = scopeProvider.CreateScope()) + { + var realOuterScope = (Scope) outerscope; + outerscope.WriteLock(Constants.Locks.ContentTree); + outerscope.WriteLock(Constants.Locks.ContentTree); + Assert.AreEqual(2, realOuterScope.WriteLocks[Constants.Locks.ContentTree]); + + using (var innerScope = scopeProvider.CreateScope()) + { + innerScope.WriteLock(Constants.Locks.ContentTree); + innerScope.WriteLock(Constants.Locks.ContentTree); + Assert.AreEqual(4, realOuterScope.WriteLocks[Constants.Locks.ContentTree]); + + innerScope.WriteLock(Constants.Locks.Languages); + innerScope.WriteLock(Constants.Locks.Languages); + Assert.AreEqual(2, realOuterScope.WriteLocks[Constants.Locks.Languages]); + innerScope.Complete(); + } + Assert.AreEqual(0, realOuterScope.WriteLocks[Constants.Locks.Languages]); + Assert.AreEqual(2, realOuterScope.WriteLocks[Constants.Locks.ContentTree]); + outerscope.Complete(); + } + } + + [Test] + public void ReadLocks_Count_correctly_If_Lock_Requested_Twice_In_Scope() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var outerscope = scopeProvider.CreateScope()) + { + var realOuterScope = (Scope) outerscope; + outerscope.ReadLock(Constants.Locks.ContentTree); + outerscope.ReadLock(Constants.Locks.ContentTree); + Assert.AreEqual(2, realOuterScope.ReadLocks[Constants.Locks.ContentTree]); + + using (var innerScope = scopeProvider.CreateScope()) + { + innerScope.ReadLock(Constants.Locks.ContentTree); + innerScope.ReadLock(Constants.Locks.ContentTree); + Assert.AreEqual(4, realOuterScope.ReadLocks[Constants.Locks.ContentTree]); + + innerScope.ReadLock(Constants.Locks.Languages); + innerScope.ReadLock(Constants.Locks.Languages); + Assert.AreEqual(2, realOuterScope.ReadLocks[Constants.Locks.Languages]); + innerScope.Complete(); + } + Assert.AreEqual(0, realOuterScope.ReadLocks[Constants.Locks.Languages]); + Assert.AreEqual(2, realOuterScope.ReadLocks[Constants.Locks.ContentTree]); + outerscope.Complete(); + } + } + [Test] public void Nested_Scopes_WriteLocks_Count_Correctly() { From c01fab97b82761619aa3b62322fa8df05b6b7397 Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 2 Mar 2021 13:43:34 +0100 Subject: [PATCH 5/9] Count requested locks in child scopes with the existing dictionaries. This way we don't have to have a two dictionaries AND two lists. --- src/Umbraco.Core/Scoping/Scope.cs | 110 ++++++++++++++++++------------ 1 file changed, 68 insertions(+), 42 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index f04d827209be..bab40a7adf70 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -35,15 +35,11 @@ internal class Scope : IScope2 private ICompletable _fscope; private IEventDispatcher _eventDispatcher; - // ReadLocks and WriteLocks owned by the entire chain, managed by the outermost scope + // ReadLocks and WriteLocks if we're the outer most scope it's those owned by the entire chain + // If we're a child scope it's those that we have requested. public Dictionary ReadLocks; public Dictionary WriteLocks; - // ReadLocks and WriteLocks requested by this specific scope, required to be able to decrement - // Using Lists just in case someone for some reason requests the same lock twice in the same scope. - private List _readLocks; - private List _writelocks; - // initializes a new scope private Scope(ScopeProvider scopeProvider, ILogger logger, FileSystems fileSystems, Scope parent, ScopeContext scopeContext, bool detachable, @@ -70,8 +66,6 @@ private Scope(ScopeProvider scopeProvider, ReadLocks = new Dictionary(); WriteLocks = new Dictionary(); - _readLocks = new List(); - _writelocks = new List(); #if DEBUG_SCOPES _scopeProvider.RegisterScope(this); @@ -363,15 +357,18 @@ public void Dispose() #endif } - // Decrement the lock counters - foreach (var readLockId in _readLocks) + // Decrement the lock counters on the parent if any. + if (ParentScope != null) { - DecrementReadLock(readLockId); - } + foreach (var readLockPair in ReadLocks) + { + DecrementReadLock(readLockPair.Key, readLockPair.Value); + } - foreach (var writeLockId in _writelocks) - { - DecrementWriteLock(writeLockId); + foreach (var writeLockPair in WriteLocks) + { + DecrementWriteLock(writeLockPair.Key, writeLockPair.Value); + } } var parent = ParentScope; @@ -516,80 +513,109 @@ private static void TryFinally(int index, Action[] actions) /// Decrements the count of the ReadLocks with a specific lock object identifier we currently hold /// /// Lock object identifier to decrement - public void DecrementReadLock(int lockId) + /// Amount to decrement the lock count with + public void DecrementReadLock(int lockId, int amountToDecrement) { // If we aren't the outermost scope, pass it on to the parent. if (ParentScope != null) { - ParentScope.DecrementReadLock(lockId); + ParentScope.DecrementReadLock(lockId, amountToDecrement); return; } - ReadLocks[lockId] -= 1; + ReadLocks[lockId] -= amountToDecrement; } /// /// Decrements the count of the WriteLocks with a specific lock object identifier we currently hold. /// /// Lock object identifier to decrement. - public void DecrementWriteLock(int lockId) + /// Amount to decrement the lock count with + public void DecrementWriteLock(int lockId, int amountToDecrement) { // If we aren't the outermost scope, pass it on to the parent. if (ParentScope != null) { - ParentScope.DecrementWriteLock(lockId); + ParentScope.DecrementWriteLock(lockId, amountToDecrement); return; } - WriteLocks[lockId] -= 1; + WriteLocks[lockId] -= amountToDecrement; } - /// - public void ReadLock(params int[] lockIds) + /// + /// Increment the count of the read locks we've requested + /// + /// + /// This should only be done on child scopes since it's then used to decrement the count later. + /// + /// + private void IncrementRequestedReadLock(params int[] lockIds) { + // We need to keep track of what lockIds we have requested locks for to be able to decrement them. if (ParentScope != null) { - foreach (var id in lockIds) + foreach (var lockId in lockIds) { - // We need to keep track of what lockIds we have requested locks for to be able to decrement them. - // We have to do this out of the recursion to not add the ids to all scopes. - _readLocks.Add(id); + if (!ReadLocks.ContainsKey(lockId)) + { + ReadLocks[lockId] = 0; + } + + ReadLocks[lockId] += 1; } } - ReadLockInner(null, lockIds); } - /// - public void ReadLock(TimeSpan timeout, int lockId) + /// + /// Increment the count of the write locks we've requested + /// + /// + /// This should only be done on child scopes since it's then used to decrement the count later. + /// + /// + private void IncrementRequestedWriteLock(params int[] lockIds) { + // We need to keep track of what lockIds we have requested locks for to be able to decrement them. if (ParentScope != null) { - _readLocks.Add(lockId); + foreach (var lockId in lockIds) + { + if (!WriteLocks.ContainsKey(lockId)) + { + WriteLocks[lockId] = 0; + } + + WriteLocks[lockId] += 1; + } } + } + + /// + public void ReadLock(params int[] lockIds) + { + IncrementRequestedReadLock(lockIds); + ReadLockInner(null, lockIds); + } + /// + public void ReadLock(TimeSpan timeout, int lockId) + { + IncrementRequestedReadLock(lockId); ReadLockInner(timeout, lockId); } /// public void WriteLock(params int[] lockIds) { - if (ParentScope != null) - { - foreach (var lockId in lockIds) - { - _writelocks.Add(lockId); - } - } + IncrementRequestedWriteLock(lockIds); WriteLockInner(null, lockIds); } /// public void WriteLock(TimeSpan timeout, int lockId) { - if (ParentScope != null) - { - _writelocks.Add(lockId); - } + IncrementRequestedWriteLock(lockId); WriteLockInner(timeout, lockId); } From f0723870962b70f81c76d8029e4c30def2ba3f59 Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 3 Mar 2021 08:55:14 +0100 Subject: [PATCH 6/9] Wrap access to dictionaries in locks and make dictionaries internal readonly --- src/Umbraco.Core/Scoping/Scope.cs | 113 ++++++++++++++++++------------ 1 file changed, 70 insertions(+), 43 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index bab40a7adf70..f1827d7d83f2 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -35,10 +35,12 @@ internal class Scope : IScope2 private ICompletable _fscope; private IEventDispatcher _eventDispatcher; + private object _dictionaryLocker; + // ReadLocks and WriteLocks if we're the outer most scope it's those owned by the entire chain // If we're a child scope it's those that we have requested. - public Dictionary ReadLocks; - public Dictionary WriteLocks; + internal readonly Dictionary ReadLocks; + internal readonly Dictionary WriteLocks; // initializes a new scope private Scope(ScopeProvider scopeProvider, @@ -64,6 +66,7 @@ private Scope(ScopeProvider scopeProvider, Detachable = detachable; + _dictionaryLocker = new object(); ReadLocks = new Dictionary(); WriteLocks = new Dictionary(); @@ -360,14 +363,20 @@ public void Dispose() // Decrement the lock counters on the parent if any. if (ParentScope != null) { - foreach (var readLockPair in ReadLocks) + lock (_dictionaryLocker) { - DecrementReadLock(readLockPair.Key, readLockPair.Value); + foreach (var readLockPair in ReadLocks) + { + DecrementReadLock(readLockPair.Key, readLockPair.Value); + } } - foreach (var writeLockPair in WriteLocks) + lock (_dictionaryLocker) { - DecrementWriteLock(writeLockPair.Key, writeLockPair.Value); + foreach (var writeLockPair in WriteLocks) + { + DecrementWriteLock(writeLockPair.Key, writeLockPair.Value); + } } } @@ -523,7 +532,10 @@ public void DecrementReadLock(int lockId, int amountToDecrement) return; } - ReadLocks[lockId] -= amountToDecrement; + lock (_dictionaryLocker) + { + ReadLocks[lockId] -= amountToDecrement; + } } /// @@ -540,7 +552,10 @@ public void DecrementWriteLock(int lockId, int amountToDecrement) return; } - WriteLocks[lockId] -= amountToDecrement; + lock (_dictionaryLocker) + { + WriteLocks[lockId] -= amountToDecrement; + } } /// @@ -557,12 +572,15 @@ private void IncrementRequestedReadLock(params int[] lockIds) { foreach (var lockId in lockIds) { - if (!ReadLocks.ContainsKey(lockId)) + lock (_dictionaryLocker) { - ReadLocks[lockId] = 0; - } + if (!ReadLocks.ContainsKey(lockId)) + { + ReadLocks[lockId] = 0; + } - ReadLocks[lockId] += 1; + ReadLocks[lockId] += 1; + } } } } @@ -581,12 +599,15 @@ private void IncrementRequestedWriteLock(params int[] lockIds) { foreach (var lockId in lockIds) { - if (!WriteLocks.ContainsKey(lockId)) + lock (_dictionaryLocker) { - WriteLocks[lockId] = 0; - } + if (!WriteLocks.ContainsKey(lockId)) + { + WriteLocks[lockId] = 0; + } - WriteLocks[lockId] += 1; + WriteLocks[lockId] += 1; + } } } } @@ -636,24 +657,27 @@ internal void ReadLockInner(TimeSpan? timeout = null, params int[] lockIds) // If we are the parent, then handle the lock request. foreach (var lockId in lockIds) { - // Only acquire the lock if we haven't done so yet. - if (!ReadLocks.ContainsKey(lockId)) + lock (_dictionaryLocker) { - if (timeout is null) - { - // We want a lock with a custom timeout - ObtainReadLock(lockId); - } - else + // Only acquire the lock if we haven't done so yet. + if (!ReadLocks.ContainsKey(lockId)) { - // We just want an ordinary lock. - ObtainTimoutReadLock(lockId, timeout.Value); + if (timeout is null) + { + // We want a lock with a custom timeout + ObtainReadLock(lockId); + } + else + { + // We just want an ordinary lock. + ObtainTimoutReadLock(lockId, timeout.Value); + } + // Add the lockId as a key to the dict. + ReadLocks[lockId] = 0; } - // Add the lockId as a key to the dict. - ReadLocks[lockId] = 0; - } - ReadLocks[lockId] += 1; + ReadLocks[lockId] += 1; + } } } @@ -673,23 +697,26 @@ internal void WriteLockInner(TimeSpan? timeout = null, params int[] lockIds) foreach (var lockId in lockIds) { - // Only acquire lock if we haven't yet (WriteLocks not containing the key) - if (!WriteLocks.ContainsKey(lockId)) + lock (_dictionaryLocker) { - if (timeout is null) - { - ObtainWriteLock(lockId); - } - else + // Only acquire lock if we haven't yet (WriteLocks not containing the key) + if (!WriteLocks.ContainsKey(lockId)) { - ObtainTimeoutWriteLock(lockId, timeout.Value); + if (timeout is null) + { + ObtainWriteLock(lockId); + } + else + { + ObtainTimeoutWriteLock(lockId, timeout.Value); + } + // Add the lockId as a key to the dict. + WriteLocks[lockId] = 0; } - // Add the lockId as a key to the dict. - WriteLocks[lockId] = 0; - } - // Increment count of the lock by 1. - WriteLocks[lockId] += 1; + // Increment count of the lock by 1. + WriteLocks[lockId] += 1; + } } } From 6898fd2784393f1d6523cd9f1156aff404893a3b Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 9 Mar 2021 08:19:40 +0100 Subject: [PATCH 7/9] Only lock once when decrementing --- src/Umbraco.Core/Scoping/Scope.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index f1827d7d83f2..0b100861544a 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -369,10 +369,7 @@ public void Dispose() { DecrementReadLock(readLockPair.Key, readLockPair.Value); } - } - lock (_dictionaryLocker) - { foreach (var writeLockPair in WriteLocks) { DecrementWriteLock(writeLockPair.Key, writeLockPair.Value); From b554aa9484af83f3c17322d14c010142b13c3796 Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 9 Mar 2021 08:22:16 +0100 Subject: [PATCH 8/9] Apply suggestions from code review Co-authored-by: Bjarke Berg --- src/Umbraco.Core/Scoping/Scope.cs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 0b100861544a..819bc515edae 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -571,12 +571,14 @@ private void IncrementRequestedReadLock(params int[] lockIds) { lock (_dictionaryLocker) { - if (!ReadLocks.ContainsKey(lockId)) + if (ReadLocks.ContainsKey(lockId)) { - ReadLocks[lockId] = 0; + ReadLocks[lockId] += 1; + } + else + { + ReadLocks[lockId] = 1; } - - ReadLocks[lockId] += 1; } } } @@ -598,12 +600,14 @@ private void IncrementRequestedWriteLock(params int[] lockIds) { lock (_dictionaryLocker) { - if (!WriteLocks.ContainsKey(lockId)) + if (WriteLocks.ContainsKey(lockId)) { - WriteLocks[lockId] = 0; + WriteLocks[lockId] = 1; + } + else + { + WriteLocks[lockId] += 1; } - - WriteLocks[lockId] += 1; } } } From adf504c20ce9fcf73ab014e6a314230153798fea Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 9 Mar 2021 09:33:34 +0100 Subject: [PATCH 9/9] Fix IncrementRequestedWriteLock --- src/Umbraco.Core/Scoping/Scope.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 819bc515edae..7015cee5ebd0 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -602,11 +602,11 @@ private void IncrementRequestedWriteLock(params int[] lockIds) { if (WriteLocks.ContainsKey(lockId)) { - WriteLocks[lockId] = 1; + WriteLocks[lockId] += 1; } else { - WriteLocks[lockId] += 1; + WriteLocks[lockId] = 1; } } }