From 498a5e0c661847eecdc130b19d8f8cecce103bec Mon Sep 17 00:00:00 2001 From: Mole Date: Thu, 11 Mar 2021 15:52:14 +0100 Subject: [PATCH 01/14] Initial rework of Lock dictionaries --- src/Umbraco.Core/Scoping/Scope.cs | 258 ++++++++++++++++-------------- 1 file changed, 134 insertions(+), 124 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 7015cee5eb..91a229ef79 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Data; +using System.Linq; using Umbraco.Core.Cache; using Umbraco.Core.Composing; using Umbraco.Core.Events; @@ -39,8 +40,8 @@ namespace Umbraco.Core.Scoping // 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. - internal readonly Dictionary ReadLocks; - internal readonly Dictionary WriteLocks; + internal readonly Dictionary> ReadLocks; + internal readonly Dictionary> WriteLocks; // initializes a new scope private Scope(ScopeProvider scopeProvider, @@ -67,8 +68,8 @@ namespace Umbraco.Core.Scoping Detachable = detachable; _dictionaryLocker = new object(); - ReadLocks = new Dictionary(); - WriteLocks = new Dictionary(); + ReadLocks = new Dictionary>(); + WriteLocks = new Dictionary>(); #if DEBUG_SCOPES _scopeProvider.RegisterScope(this); @@ -361,18 +362,17 @@ namespace Umbraco.Core.Scoping } // Decrement the lock counters on the parent if any. - if (ParentScope != null) + lock (_dictionaryLocker) { - lock (_dictionaryLocker) - { - foreach (var readLockPair in ReadLocks) - { - DecrementReadLock(readLockPair.Key, readLockPair.Value); - } + ClearReadLocks(InstanceId); + ClearWriteLocks(InstanceId); - foreach (var writeLockPair in WriteLocks) + if (ParentScope is null) + { + // We're the parent scope, make sure that locks of all scopes has been cleared + if (ReadLocks.Count != 0 || WriteLocks.Count != 0) { - DecrementWriteLock(writeLockPair.Key, writeLockPair.Value); + throw new Exception($"All scopes has not been disposed from parent scope {InstanceId}"); } } } @@ -515,130 +515,122 @@ namespace Umbraco.Core.Scoping private static bool LogUncompletedScopes => (_logUncompletedScopes ?? (_logUncompletedScopes = Current.Configs.CoreDebug().LogUncompletedScopes)).Value; - /// - /// Decrements the count of the ReadLocks with a specific lock object identifier we currently hold - /// - /// Lock object identifier to decrement - /// Amount to decrement the lock count with - public void DecrementReadLock(int lockId, int amountToDecrement) + private void IncrementWriteLock(int lockId, Guid instanceId) { - // If we aren't the outermost scope, pass it on to the parent. - if (ParentScope != null) + if (ParentScope is null) { - ParentScope.DecrementReadLock(lockId, amountToDecrement); - return; + WriteLocks[instanceId][lockId]++; } - - lock (_dictionaryLocker) + else { - ReadLocks[lockId] -= amountToDecrement; + ParentScope.IncrementWriteLock(lockId, instanceId); } } - /// - /// Decrements the count of the WriteLocks with a specific lock object identifier we currently hold. - /// - /// Lock object identifier to decrement. - /// Amount to decrement the lock count with - public void DecrementWriteLock(int lockId, int amountToDecrement) + private void DecrementWriteLock(int lockId, Guid instanceId) { - // If we aren't the outermost scope, pass it on to the parent. - if (ParentScope != null) + if (ParentScope is null) { - ParentScope.DecrementWriteLock(lockId, amountToDecrement); - return; + WriteLocks[instanceId][lockId]--; } - - lock (_dictionaryLocker) + else { - WriteLocks[lockId] -= amountToDecrement; + ParentScope.DecrementWriteLock(lockId, instanceId); } } - /// - /// 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) + private void IncrementReadLock(int lockId, Guid instanceId) { - // We need to keep track of what lockIds we have requested locks for to be able to decrement them. - if (ParentScope != null) + if (ParentScope is null) { - foreach (var lockId in lockIds) - { - lock (_dictionaryLocker) - { - if (ReadLocks.ContainsKey(lockId)) - { - ReadLocks[lockId] += 1; - } - else - { - ReadLocks[lockId] = 1; - } - } - } + ReadLocks[instanceId][lockId]++; + } + else + { + ParentScope.IncrementReadLock(lockId, instanceId); } } - /// - /// 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) + private void DecrementReadLock(int lockId, Guid instanceId) + { + if (ParentScope is null) + { + ReadLocks[instanceId][lockId]--; + } + else + { + ParentScope.DecrementReadLock(lockId, instanceId); + } + } + + private void ClearReadLocks(Guid instanceId) { - // We need to keep track of what lockIds we have requested locks for to be able to decrement them. if (ParentScope != null) { - foreach (var lockId in lockIds) - { - lock (_dictionaryLocker) - { - if (WriteLocks.ContainsKey(lockId)) - { - WriteLocks[lockId] += 1; - } - else - { - WriteLocks[lockId] = 1; - } - } - } + ParentScope.ClearReadLocks(instanceId); + } + else + { + ReadLocks.Remove(instanceId); + } + } + + private void ClearWriteLocks(Guid instanceID) + { + if (ParentScope != null) + { + ParentScope.ClearWriteLocks(instanceID); + } + else + { + WriteLocks.Remove(instanceID); } } /// public void ReadLock(params int[] lockIds) { - IncrementRequestedReadLock(lockIds); - ReadLockInner(null, lockIds); + ReadLockInner(InstanceId, null, lockIds); } /// public void ReadLock(TimeSpan timeout, int lockId) { - IncrementRequestedReadLock(lockId); - ReadLockInner(timeout, lockId); + ReadLockInner(InstanceId, timeout, lockId); } /// public void WriteLock(params int[] lockIds) { - IncrementRequestedWriteLock(lockIds); - WriteLockInner(null, lockIds); + WriteLockInner(InstanceId, null, lockIds); } /// public void WriteLock(TimeSpan timeout, int lockId) { - IncrementRequestedWriteLock(lockId); - WriteLockInner(timeout, lockId); + WriteLockInner(InstanceId, timeout, lockId); + } + + /// + /// Determine if a read lock with the specified ID has already been obtained. + /// + /// Id to test. + /// True if no scopes has obtained a read lock with the specific ID yet. + private bool HasReadLock(int lockId) + { + // Check if there is any dictionary with a key equal to lockId + // And check that the value associated with that key is greater than 0, if not it could be because a lock was requested but it failed. + return ReadLocks.Values.Where(x => x.ContainsKey(lockId)).Any(x => x[lockId] > 0); + } + + /// + /// Determine if a write lock with the specified ID has already been obtained. + /// + /// Id to test + /// >True if no scopes has obtained a write lock with the specific ID yet. + private bool HasWriteLock(int lockId) + { + return WriteLocks.Values.Where(x => x.ContainsKey(lockId)).Any(x => x[lockId] > 0); } /// @@ -646,38 +638,49 @@ namespace Umbraco.Core.Scoping /// /// Optional database timeout in milliseconds. /// Array of lock object identifiers. - internal void ReadLockInner(TimeSpan? timeout = null, params int[] lockIds) + private void ReadLockInner(Guid instanceId, TimeSpan? timeout = null, params int[] lockIds) { if (ParentScope != null) { // Delegate acquiring the lock to the parent if any. - ParentScope.ReadLockInner(timeout, lockIds); + ParentScope.ReadLockInner(instanceId, timeout, lockIds); return; } - // If we are the parent, then handle the lock request. - foreach (var lockId in lockIds) + lock (_dictionaryLocker) { - lock (_dictionaryLocker) + // 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)) + if (!HasReadLock(lockId)) { - if (timeout is null) + IncrementReadLock(lockId, instanceId); + try { - // We want a lock with a custom timeout - ObtainReadLock(lockId); + if (timeout is null) + { + // We just want an ordinary lock. + ObtainReadLock(lockId); + } + else + { + // We want a lock with a custom timeout + ObtainTimoutReadLock(lockId, timeout.Value); + } } - else + catch { - // We just want an ordinary lock. - ObtainTimoutReadLock(lockId, timeout.Value); + // Something went wrong and we didn't get the lock, decrement the count and throw. + DecrementReadLock(lockId, instanceId); + throw; } - // Add the lockId as a key to the dict. - ReadLocks[lockId] = 0; } - - ReadLocks[lockId] += 1; + else + { + // We already have a lock, but need to update the readlock dictionary for debugging purposes. + IncrementReadLock(lockId, instanceId); + } } } } @@ -687,36 +690,43 @@ namespace Umbraco.Core.Scoping /// /// Optional database timeout in milliseconds. /// Array of lock object identifiers. - internal void WriteLockInner(TimeSpan? timeout = null, params int[] lockIds) + internal void WriteLockInner(Guid instanceId, TimeSpan? timeout = null, params int[] lockIds) { if (ParentScope != null) { // If we have a parent we delegate lock creation to parent. - ParentScope.WriteLockInner(timeout, lockIds); + ParentScope.WriteLockInner(instanceId, timeout, lockIds); return; } - foreach (var lockId in lockIds) + lock (_dictionaryLocker) { - lock (_dictionaryLocker) + foreach (var lockId in lockIds) { // Only acquire lock if we haven't yet (WriteLocks not containing the key) - if (!WriteLocks.ContainsKey(lockId)) + if (!HasWriteLock(lockId)) { - if (timeout is null) + IncrementWriteLock(lockId, instanceId); + try { - ObtainWriteLock(lockId); + if (timeout is null) + { + ObtainWriteLock(lockId); + } + else + { + ObtainTimeoutWriteLock(lockId, timeout.Value); + } } - else + catch { - ObtainTimeoutWriteLock(lockId, timeout.Value); + DecrementWriteLock(lockId, instanceId); } - // Add the lockId as a key to the dict. - WriteLocks[lockId] = 0; } - - // Increment count of the lock by 1. - WriteLocks[lockId] += 1; + else + { + DecrementWriteLock(lockId, instanceId); + } } } } From 5c7e4f8ddebedac951ca68f27156be0eae8852bb Mon Sep 17 00:00:00 2001 From: Mole Date: Fri, 12 Mar 2021 13:10:17 +0100 Subject: [PATCH 02/14] Adjust unit tests and apply fixes to scope --- src/Umbraco.Core/Scoping/Scope.cs | 45 ++++++- src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 134 ++++++++++++-------- 2 files changed, 122 insertions(+), 57 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 91a229ef79..d9846779eb 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -519,7 +519,26 @@ namespace Umbraco.Core.Scoping { if (ParentScope is null) { - WriteLocks[instanceId][lockId]++; + // Try and get the dict associated with the scope id. + var locksDictFound = WriteLocks.TryGetValue(instanceId, out var locksDict); + if (locksDictFound) + { + var lockFound = locksDict.TryGetValue(lockId, out var value); + if (lockFound) + { + WriteLocks[instanceId][lockId] = value+1; + } + else + { + WriteLocks[instanceId][lockId] = 1; + } + } + else + { + // The scope hasn't requested a lock yet, so we have to create a dict for it. + WriteLocks[instanceId] = new Dictionary(); + WriteLocks[instanceId][lockId] = 1; + } } else { @@ -543,7 +562,25 @@ namespace Umbraco.Core.Scoping { if (ParentScope is null) { - ReadLocks[instanceId][lockId]++; + var locksDictFound = ReadLocks.TryGetValue(instanceId, out var locksDict); + if (locksDictFound) + { + var lockFound = locksDict.TryGetValue(lockId, out var value); + if (lockFound) + { + ReadLocks[instanceId][lockId] = value + 1; + } + else + { + ReadLocks[instanceId][lockId] = 1; + } + } + else + { + // The scope hasn't requested a lock yet, so we have to create a dict for it. + ReadLocks[instanceId] = new Dictionary(); + ReadLocks[instanceId][lockId] = 1; + } } else { @@ -721,11 +758,13 @@ namespace Umbraco.Core.Scoping catch { DecrementWriteLock(lockId, instanceId); + throw; } } else { - DecrementWriteLock(lockId, instanceId); + // We already have a lock, so just increment the count for debugging purposes + IncrementWriteLock(lockId, instanceId); } } } diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index 32bd7e2afe..c222769abb 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -180,27 +180,30 @@ namespace Umbraco.Tests.Scoping public void WriteLocks_Count_correctly_If_Lock_Requested_Twice_In_Scope() { var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + Guid innerscopeId; 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]); + Assert.AreEqual(2, realOuterScope.WriteLocks[outerscope.InstanceId][Constants.Locks.ContentTree]); using (var innerScope = scopeProvider.CreateScope()) { + innerscopeId = innerScope.InstanceId; innerScope.WriteLock(Constants.Locks.ContentTree); innerScope.WriteLock(Constants.Locks.ContentTree); - Assert.AreEqual(4, realOuterScope.WriteLocks[Constants.Locks.ContentTree]); + Assert.AreEqual(2, realOuterScope.WriteLocks[outerscope.InstanceId][Constants.Locks.ContentTree]); + Assert.AreEqual(2, realOuterScope.WriteLocks[innerscopeId][Constants.Locks.ContentTree]); innerScope.WriteLock(Constants.Locks.Languages); innerScope.WriteLock(Constants.Locks.Languages); - Assert.AreEqual(2, realOuterScope.WriteLocks[Constants.Locks.Languages]); + Assert.AreEqual(2, realOuterScope.WriteLocks[innerScope.InstanceId][Constants.Locks.Languages]); innerScope.Complete(); } - Assert.AreEqual(0, realOuterScope.WriteLocks[Constants.Locks.Languages]); - Assert.AreEqual(2, realOuterScope.WriteLocks[Constants.Locks.ContentTree]); + Assert.IsFalse(realOuterScope.WriteLocks.ContainsKey(innerscopeId)); + Assert.AreEqual(2, realOuterScope.WriteLocks[realOuterScope.InstanceId][Constants.Locks.ContentTree]); outerscope.Complete(); } } @@ -209,27 +212,30 @@ namespace Umbraco.Tests.Scoping public void ReadLocks_Count_correctly_If_Lock_Requested_Twice_In_Scope() { var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + Guid innerscopeId; 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]); + Assert.AreEqual(2, realOuterScope.ReadLocks[outerscope.InstanceId][Constants.Locks.ContentTree]); using (var innerScope = scopeProvider.CreateScope()) { + innerscopeId = innerScope.InstanceId; innerScope.ReadLock(Constants.Locks.ContentTree); innerScope.ReadLock(Constants.Locks.ContentTree); - Assert.AreEqual(4, realOuterScope.ReadLocks[Constants.Locks.ContentTree]); + Assert.AreEqual(2, realOuterScope.ReadLocks[outerscope.InstanceId][Constants.Locks.ContentTree]); + Assert.AreEqual(2, realOuterScope.ReadLocks[innerScope.InstanceId][Constants.Locks.ContentTree]); innerScope.ReadLock(Constants.Locks.Languages); innerScope.ReadLock(Constants.Locks.Languages); - Assert.AreEqual(2, realOuterScope.ReadLocks[Constants.Locks.Languages]); + Assert.AreEqual(2, realOuterScope.ReadLocks[innerScope.InstanceId][Constants.Locks.Languages]); innerScope.Complete(); } - Assert.AreEqual(0, realOuterScope.ReadLocks[Constants.Locks.Languages]); - Assert.AreEqual(2, realOuterScope.ReadLocks[Constants.Locks.ContentTree]); + Assert.IsFalse(realOuterScope.ReadLocks.ContainsKey(innerscopeId)); + Assert.AreEqual(2, realOuterScope.ReadLocks[outerscope.InstanceId][Constants.Locks.ContentTree]); outerscope.Complete(); } } @@ -238,51 +244,60 @@ namespace Umbraco.Tests.Scoping public void Nested_Scopes_WriteLocks_Count_Correctly() { var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + Guid innerScope1Id, innerScope2Id; - using (var outerScope = scopeProvider.CreateScope()) + using (var parentScope = scopeProvider.CreateScope()) { - var parentScope = (Scope) outerScope; - outerScope.WriteLock(Constants.Locks.ContentTree); - outerScope.WriteLock(Constants.Locks.ContentTypes); + var realParentScope = (Scope) parentScope; + parentScope.WriteLock(Constants.Locks.ContentTree); + parentScope.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)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); using (var innerScope1 = scopeProvider.CreateScope()) { + innerScope1Id = innerScope1.InstanceId; 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)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"innerScope1, parent instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, parent instance, after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope1, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.Languages], $"innerScope1, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.Languages)}"); using (var innerScope2 = scopeProvider.CreateScope()) { + innerScope2Id = innerScope2.InstanceId; 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)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"innerScope2, parent instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"innerScope2, parent instance, after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope2, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope2, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.Languages], $"innerScope2, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope2.InstanceId][Constants.Locks.ContentTree], $"innerScope2, innerScope2 instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope2.InstanceId][Constants.Locks.MediaTypes], $"innerScope2, innerScope2 instance, 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)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"innerScope2, parent instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"innerScope2, parent instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope2, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope2, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.Languages], $"innerScope2, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.Languages)}"); + Assert.IsFalse(realParentScope.WriteLocks.ContainsKey(innerScope2Id)); 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)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed {nameof(Constants.Locks.ContentTypes)}"); + Assert.IsFalse(realParentScope.WriteLocks.ContainsKey(innerScope1Id)); - outerScope.Complete(); + parentScope.Complete(); } } @@ -290,48 +305,59 @@ namespace Umbraco.Tests.Scoping public void Nested_Scopes_ReadLocks_Count_Correctly() { var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + Guid innerScope1Id, innerScope2Id; - using (var outerScope = scopeProvider.CreateScope()) + using (var parentScope = 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)}"); + var realParentScope = (Scope) parentScope; + parentScope.ReadLock(Constants.Locks.ContentTree); + parentScope.ReadLock(Constants.Locks.ContentTypes); + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); using (var innserScope1 = scopeProvider.CreateScope()) { + innerScope1Id = innserScope1.InstanceId; 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)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"innerScope1, parent instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, parent instance, after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope1, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.Languages], $"innerScope1, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.Languages)}"); using (var innerScope2 = scopeProvider.CreateScope()) { + innerScope2Id = innerScope2.InstanceId; 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)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"innerScope2, parent instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"innerScope2, parent instance, after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope2, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope2, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.Languages], $"innerScope2, innerScope1 instance, after locks acquired: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innerScope2.InstanceId][Constants.Locks.ContentTree], $"innerScope2, innerScope2 instance, after locks acquired: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innerScope2.InstanceId][Constants.Locks.MediaTypes], $"innerScope2, innerScope2 instance, 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)}"); + + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"innerScope1, parent instance, after innerScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, parent instance, after innerScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope1, innerScope1 instance, after innerScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, innerScope1 instance, after innerScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.Languages], $"innerScope1, innerScope1 instance, after innerScope2 disposed: {nameof(Constants.Locks.Languages)}"); + Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope2Id)); 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(); + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"parentScope after innerScope1 disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after innerScope1 disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope1Id)); + + parentScope.Complete(); } } } From ea816e8110f1c20a7e910774ba688683780e9ed7 Mon Sep 17 00:00:00 2001 From: Mole Date: Fri, 12 Mar 2021 13:30:25 +0100 Subject: [PATCH 03/14] Add more unit tests, showing current issue --- src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 82 +++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index c222769abb..3e5ff127a3 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -72,6 +72,31 @@ namespace Umbraco.Tests.Scoping syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.Languages), Times.Once); } + [Test] + public void WriteLock_Acquired_Only_Once_When_InnerScope_Disposed() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var outerScope = scopeProvider.CreateScope()) + { + outerScope.WriteLock(Constants.Locks.Languages); + + using (var innerScope = scopeProvider.CreateScope()) + { + innerScope.WriteLock(Constants.Locks.Languages); + innerScope.WriteLock(Constants.Locks.ContentTree); + innerScope.Complete(); + } + + outerScope.WriteLock(Constants.Locks.ContentTree); + outerScope.Complete(); + } + + // Since we request the ReadLock after the innerScope has been dispose, the key has been removed from the dictionary, and we fetch it again. + syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.Languages), Times.Once); + syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.ContentTree), Times.Once); + } + [Test] public void WriteLock_With_Timeout_Acquired_Only_Once_Per_Key(){ var scopeProvider = GetScopeProvider(out var syntaxProviderMock); @@ -176,6 +201,31 @@ namespace Umbraco.Tests.Scoping syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), timeOut, Constants.Locks.Languages), Times.Once); } + [Test] + public void ReadLock_Acquired_Only_Once_When_InnerScope_Disposed() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var outerScope = scopeProvider.CreateScope()) + { + outerScope.ReadLock(Constants.Locks.Languages); + + using (var innerScope = scopeProvider.CreateScope()) + { + innerScope.ReadLock(Constants.Locks.Languages); + innerScope.ReadLock(Constants.Locks.ContentTree); + innerScope.Complete(); + } + + outerScope.ReadLock(Constants.Locks.ContentTree); + outerScope.Complete(); + } + + // Since we request the ReadLock after the innerScope has been dispose, the key has been removed from the dictionary, and we fetch it again. + syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.Languages), Times.Once); + syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.ContentTree), Times.Once); + } + [Test] public void WriteLocks_Count_correctly_If_Lock_Requested_Twice_In_Scope() { @@ -360,5 +410,37 @@ namespace Umbraco.Tests.Scoping parentScope.Complete(); } } + + [Test] + public void WriteLock_Doesnt_Increment_On_Error() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + syntaxProviderMock.Setup(x => x.WriteLock(It.IsAny(), It.IsAny())).Throws(new Exception("Boom")); + + using (var scope = scopeProvider.CreateScope()) + { + var realScope = (Scope) scope; + + Assert.Throws(() => scope.WriteLock(Constants.Locks.Languages)); + Assert.AreEqual(0, realScope.WriteLocks[scope.InstanceId][Constants.Locks.Languages]); + scope.Complete(); + } + } + + [Test] + public void ReadLock_Doesnt_Increment_On_Error() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + syntaxProviderMock.Setup(x => x.ReadLock(It.IsAny(), It.IsAny())).Throws(new Exception("Boom")); + + using (var scope = scopeProvider.CreateScope()) + { + var realScope = (Scope) scope; + + Assert.Throws(() => scope.ReadLock(Constants.Locks.Languages)); + Assert.AreEqual(0, realScope.ReadLocks[scope.InstanceId][Constants.Locks.Languages]); + scope.Complete(); + } + } } } From cee33e863ef29fc81c69bf18f9e75421beac87d3 Mon Sep 17 00:00:00 2001 From: Mole Date: Mon, 15 Mar 2021 08:38:23 +0100 Subject: [PATCH 04/14] Fix errors shown in unit tests --- src/Umbraco.Core/Scoping/Scope.cs | 87 ++++++++++----------- src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 44 +++++++---- 2 files changed, 70 insertions(+), 61 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index d9846779eb..3cff2da020 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -362,20 +362,18 @@ namespace Umbraco.Core.Scoping } // Decrement the lock counters on the parent if any. - lock (_dictionaryLocker) - { - ClearReadLocks(InstanceId); - ClearWriteLocks(InstanceId); - - if (ParentScope is null) - { - // We're the parent scope, make sure that locks of all scopes has been cleared - if (ReadLocks.Count != 0 || WriteLocks.Count != 0) - { - throw new Exception($"All scopes has not been disposed from parent scope {InstanceId}"); - } - } - } + // Lock on parent + ClearReadLocks(InstanceId); + ClearWriteLocks(InstanceId); + // if (ParentScope is null) + // { + // // We're the parent scope, make sure that locks of all scopes has been cleared + // // Since we're only reading we don't have to be in a lock + // if (ReadLocks.Count != 0 || WriteLocks.Count != 0) + // { + // throw new Exception($"All scopes has not been disposed from parent scope {InstanceId}"); + // } + // } var parent = ParentScope; _scopeProvider.AmbientScope = parent; // might be null = this is how scopes are removed from context objects @@ -546,18 +544,6 @@ namespace Umbraco.Core.Scoping } } - private void DecrementWriteLock(int lockId, Guid instanceId) - { - if (ParentScope is null) - { - WriteLocks[instanceId][lockId]--; - } - else - { - ParentScope.DecrementWriteLock(lockId, instanceId); - } - } - private void IncrementReadLock(int lockId, Guid instanceId) { if (ParentScope is null) @@ -588,18 +574,6 @@ namespace Umbraco.Core.Scoping } } - private void DecrementReadLock(int lockId, Guid instanceId) - { - if (ParentScope is null) - { - ReadLocks[instanceId][lockId]--; - } - else - { - ParentScope.DecrementReadLock(lockId, instanceId); - } - } - private void ClearReadLocks(Guid instanceId) { if (ParentScope != null) @@ -608,7 +582,17 @@ namespace Umbraco.Core.Scoping } else { - ReadLocks.Remove(instanceId); + lock (_dictionaryLocker) + { + // Rest all values to 0 since the scope has been disposed + if (ReadLocks.ContainsKey(instanceId)) + { + foreach (var key in ReadLocks[instanceId].Keys.ToList()) + { + ReadLocks[instanceId][key] = 0; + } + } + } } } @@ -620,7 +604,17 @@ namespace Umbraco.Core.Scoping } else { - WriteLocks.Remove(instanceID); + lock (_dictionaryLocker) + { + if (WriteLocks.ContainsKey(instanceID)) + { + foreach (var key in WriteLocks[instanceID].Keys.ToList()) + { + WriteLocks[instanceID][key] = 0; + } + } + } + } } @@ -657,7 +651,7 @@ namespace Umbraco.Core.Scoping { // Check if there is any dictionary with a key equal to lockId // And check that the value associated with that key is greater than 0, if not it could be because a lock was requested but it failed. - return ReadLocks.Values.Where(x => x.ContainsKey(lockId)).Any(x => x[lockId] > 0); + return ReadLocks.Values.Any(x => x.ContainsKey(lockId)); } /// @@ -667,7 +661,7 @@ namespace Umbraco.Core.Scoping /// >True if no scopes has obtained a write lock with the specific ID yet. private bool HasWriteLock(int lockId) { - return WriteLocks.Values.Where(x => x.ContainsKey(lockId)).Any(x => x[lockId] > 0); + return WriteLocks.Values.Any(x => x.ContainsKey(lockId)); } /// @@ -708,8 +702,9 @@ namespace Umbraco.Core.Scoping } catch { - // Something went wrong and we didn't get the lock, decrement the count and throw. - DecrementReadLock(lockId, instanceId); + // Something went wrong and we didn't get the lock + // Since we at this point have determined that we haven't got any key of LockID, it's safe to completely remove it instead of decrementing. + ReadLocks[instanceId].Remove(lockId); throw; } } @@ -757,7 +752,9 @@ namespace Umbraco.Core.Scoping } catch { - DecrementWriteLock(lockId, instanceId); + // Something went wrong and we didn't get the lock + // Since we at this point have determined that we haven't got any key of LockID, it's safe to completely remove it instead of decrementing. + WriteLocks[instanceId].Remove(lockId); throw; } } diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index 3e5ff127a3..157bfec662 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -222,8 +222,8 @@ namespace Umbraco.Tests.Scoping } // Since we request the ReadLock after the innerScope has been dispose, the key has been removed from the dictionary, and we fetch it again. - syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.Languages), Times.Once); - syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.ContentTree), Times.Once); + syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), Constants.Locks.Languages), Times.Once); + syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), Constants.Locks.ContentTree), Times.Once); } [Test] @@ -252,8 +252,9 @@ namespace Umbraco.Tests.Scoping Assert.AreEqual(2, realOuterScope.WriteLocks[innerScope.InstanceId][Constants.Locks.Languages]); innerScope.Complete(); } - Assert.IsFalse(realOuterScope.WriteLocks.ContainsKey(innerscopeId)); Assert.AreEqual(2, realOuterScope.WriteLocks[realOuterScope.InstanceId][Constants.Locks.ContentTree]); + Assert.AreEqual(0, realOuterScope.WriteLocks[innerscopeId][Constants.Locks.Languages]); + Assert.AreEqual(0, realOuterScope.WriteLocks[innerscopeId][Constants.Locks.ContentTree]); outerscope.Complete(); } } @@ -284,8 +285,11 @@ namespace Umbraco.Tests.Scoping Assert.AreEqual(2, realOuterScope.ReadLocks[innerScope.InstanceId][Constants.Locks.Languages]); innerScope.Complete(); } - Assert.IsFalse(realOuterScope.ReadLocks.ContainsKey(innerscopeId)); Assert.AreEqual(2, realOuterScope.ReadLocks[outerscope.InstanceId][Constants.Locks.ContentTree]); + Assert.AreEqual(0, realOuterScope.ReadLocks[innerscopeId][Constants.Locks.Languages]); + Assert.AreEqual(0, realOuterScope.ReadLocks[innerscopeId][Constants.Locks.ContentTree]); + + outerscope.Complete(); } } @@ -334,18 +338,23 @@ namespace Umbraco.Tests.Scoping innerScope2.Complete(); } - Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"innerScope2, parent instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"innerScope2, parent instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope2, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope2, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.Languages], $"innerScope2, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.Languages)}"); - Assert.IsFalse(realParentScope.WriteLocks.ContainsKey(innerScope2Id)); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"innerScope1, parent instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, parent instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope1, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.Languages], $"innerScope1, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.Languages)}"); + Assert.AreEqual(0, realParentScope.WriteLocks[innerScope2Id][Constants.Locks.ContentTree], $"innerScope1, innerScope2 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(0, realParentScope.WriteLocks[innerScope2Id][Constants.Locks.MediaTypes], $"innerScope1, innerScope2 instance, after innserScope2 disposed: {nameof(Constants.Locks.MediaTypes)}"); innerScope1.Complete(); } Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed {nameof(Constants.Locks.ContentTypes)}"); - Assert.IsFalse(realParentScope.WriteLocks.ContainsKey(innerScope1Id)); + Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(0, realParentScope.WriteLocks[innerScope2Id][Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(0, realParentScope.WriteLocks[innerScope2Id][Constants.Locks.MediaTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.MediaTypes)}"); + Assert.AreEqual(0, realParentScope.WriteLocks[innerScope1Id][Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); + Assert.AreEqual(0, realParentScope.WriteLocks[innerScope1Id][Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); + Assert.AreEqual(0, realParentScope.WriteLocks[innerScope1Id][Constants.Locks.Languages], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.Languages)}"); parentScope.Complete(); } @@ -398,14 +407,17 @@ namespace Umbraco.Tests.Scoping Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope1, innerScope1 instance, after innerScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, innerScope1 instance, after innerScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.Languages], $"innerScope1, innerScope1 instance, after innerScope2 disposed: {nameof(Constants.Locks.Languages)}"); - Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope2Id)); + Assert.AreEqual(0, realParentScope.ReadLocks[innerScope2Id][Constants.Locks.ContentTree]); + Assert.AreEqual(0, realParentScope.ReadLocks[innerScope2Id][Constants.Locks.MediaTypes]); innserScope1.Complete(); } Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"parentScope after innerScope1 disposed: {nameof(Constants.Locks.ContentTree)}"); Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after innerScope1 disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope1Id)); + Assert.AreEqual(0, realParentScope.ReadLocks[innerScope1Id][Constants.Locks.ContentTree]); + Assert.AreEqual(0, realParentScope.ReadLocks[innerScope1Id][Constants.Locks.ContentTypes]); + Assert.AreEqual(0, realParentScope.ReadLocks[innerScope1Id][Constants.Locks.Languages]); parentScope.Complete(); } @@ -422,7 +434,7 @@ namespace Umbraco.Tests.Scoping var realScope = (Scope) scope; Assert.Throws(() => scope.WriteLock(Constants.Locks.Languages)); - Assert.AreEqual(0, realScope.WriteLocks[scope.InstanceId][Constants.Locks.Languages]); + Assert.IsFalse(realScope.WriteLocks[scope.InstanceId].ContainsKey(Constants.Locks.Languages)); scope.Complete(); } } @@ -438,7 +450,7 @@ namespace Umbraco.Tests.Scoping var realScope = (Scope) scope; Assert.Throws(() => scope.ReadLock(Constants.Locks.Languages)); - Assert.AreEqual(0, realScope.ReadLocks[scope.InstanceId][Constants.Locks.Languages]); + Assert.IsFalse(realScope.ReadLocks[scope.InstanceId].ContainsKey(Constants.Locks.Languages)); scope.Complete(); } } From ef59829489395c5de0e4db310e562702708c79ed Mon Sep 17 00:00:00 2001 From: Mole Date: Mon, 15 Mar 2021 08:54:52 +0100 Subject: [PATCH 05/14] Throw error if all scopes hasn't been disposed --- src/Umbraco.Core/Scoping/Scope.cs | 19 +++++------ src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 35 +++++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 3cff2da020..38941086d8 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -365,15 +365,16 @@ namespace Umbraco.Core.Scoping // Lock on parent ClearReadLocks(InstanceId); ClearWriteLocks(InstanceId); - // if (ParentScope is null) - // { - // // We're the parent scope, make sure that locks of all scopes has been cleared - // // Since we're only reading we don't have to be in a lock - // if (ReadLocks.Count != 0 || WriteLocks.Count != 0) - // { - // throw new Exception($"All scopes has not been disposed from parent scope {InstanceId}"); - // } - // } + if (ParentScope is null) + { + // We're the parent scope, make sure that locks of all scopes has been cleared + // Since we're only reading we don't have to be in a lock + if (ReadLocks.Values.Any(x => x.Values.Any(value => value != 0)) + || WriteLocks.Values.Any(x => x.Values.Any(value => value != 0))) + { + throw new Exception($"All scopes has not been disposed from parent scope {InstanceId}"); + } + } var parent = ParentScope; _scopeProvider.AmbientScope = parent; // might be null = this is how scopes are removed from context objects diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index 157bfec662..0cc05bb5d5 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using Moq; using NPoco; using NUnit.Framework; @@ -454,5 +455,39 @@ namespace Umbraco.Tests.Scoping scope.Complete(); } } + + [Test] + public void Scope_Throws_If_ReadLocks_Not_Cleared() + { + var scopeprovider = GetScopeProvider(out var syntaxProviderMock); + var scope = (Scope) scopeprovider.CreateScope(); + + var readDict = new Dictionary(); + readDict[Constants.Locks.Languages] = 1; + scope.ReadLocks[Guid.NewGuid()] = readDict; + + Assert.Throws(() => scope.Dispose()); + + // We have to clear so we can properly dispose the scope, otherwise it'll mess with other tests. + scope.ReadLocks.Clear(); + scope.Dispose(); + } + + [Test] + public void Scope_Throws_If_WriteLocks_Not_Cleared() + { + var scopeprovider = GetScopeProvider(out var syntaxProviderMock); + var scope = (Scope) scopeprovider.CreateScope(); + + var writeDict = new Dictionary(); + writeDict[Constants.Locks.Languages] = 1; + scope.WriteLocks[Guid.NewGuid()] = writeDict; + + Assert.Throws(() => scope.Dispose()); + + // We have to clear so we can properly dispose the scope, otherwise it'll mess with other tests. + scope.WriteLocks.Clear(); + scope.Dispose(); + } } } From 92fd9ff6853debe5e2c9f4f38c8af0e0379ea99c Mon Sep 17 00:00:00 2001 From: Mole Date: Mon, 15 Mar 2021 11:31:23 +0100 Subject: [PATCH 06/14] Clean --- src/Umbraco.Core/Scoping/Scope.cs | 94 ++++++++++----------- src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 4 +- 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 38941086d8..4298bec93b 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -362,7 +362,6 @@ namespace Umbraco.Core.Scoping } // Decrement the lock counters on the parent if any. - // Lock on parent ClearReadLocks(InstanceId); ClearWriteLocks(InstanceId); if (ParentScope is null) @@ -372,7 +371,7 @@ namespace Umbraco.Core.Scoping if (ReadLocks.Values.Any(x => x.Values.Any(value => value != 0)) || WriteLocks.Values.Any(x => x.Values.Any(value => value != 0))) { - throw new Exception($"All scopes has not been disposed from parent scope {InstanceId}"); + throw new InvalidOperationException($"All scopes has not been disposed from parent scope: {InstanceId}"); } } @@ -514,67 +513,61 @@ namespace Umbraco.Core.Scoping private static bool LogUncompletedScopes => (_logUncompletedScopes ?? (_logUncompletedScopes = Current.Configs.CoreDebug().LogUncompletedScopes)).Value; + /// + /// Increment the counter in WriteLocks for a specific scope instance and lock identifier. + /// + /// + /// This will not lock on the lock object, ensure that you lock on _dictionaryLocker before calling this method. + /// + /// Lock ID to increment. + /// Instance ID of the scope requesting the lock. private void IncrementWriteLock(int lockId, Guid instanceId) { - if (ParentScope is null) + // Since we've already checked that we're the parent in the WriteLockInner method, we don't need to check again. + // Try and get the dict associated with the scope id. + var locksDictFound = WriteLocks.TryGetValue(instanceId, out var locksDict); + if (locksDictFound) { - // Try and get the dict associated with the scope id. - var locksDictFound = WriteLocks.TryGetValue(instanceId, out var locksDict); - if (locksDictFound) - { - var lockFound = locksDict.TryGetValue(lockId, out var value); - if (lockFound) - { - WriteLocks[instanceId][lockId] = value+1; - } - else - { - WriteLocks[instanceId][lockId] = 1; - } - } - else - { - // The scope hasn't requested a lock yet, so we have to create a dict for it. - WriteLocks[instanceId] = new Dictionary(); - WriteLocks[instanceId][lockId] = 1; - } + locksDict.TryGetValue(lockId, out var value); + WriteLocks[instanceId][lockId] = value + 1; } else { - ParentScope.IncrementWriteLock(lockId, instanceId); + // The scope hasn't requested a lock yet, so we have to create a dict for it. + WriteLocks[instanceId] = new Dictionary(); + WriteLocks[instanceId][lockId] = 1; } } + /// + /// Increment the counter in ReadLocks for a specific scope instance and lock identifier. + /// + /// + /// This will not lock on the lock object, ensure that you lock on _dictionaryLocker before calling this method. + /// + /// Lock ID to increment. + /// Instance ID of the scope requesting the lock. private void IncrementReadLock(int lockId, Guid instanceId) { - if (ParentScope is null) + // Since we've already checked that we're the parent in the WriteLockInner method, we don't need to check again. + var locksDictFound = ReadLocks.TryGetValue(instanceId, out var locksDict); + if (locksDictFound) { - var locksDictFound = ReadLocks.TryGetValue(instanceId, out var locksDict); - if (locksDictFound) - { - var lockFound = locksDict.TryGetValue(lockId, out var value); - if (lockFound) - { - ReadLocks[instanceId][lockId] = value + 1; - } - else - { - ReadLocks[instanceId][lockId] = 1; - } - } - else - { - // The scope hasn't requested a lock yet, so we have to create a dict for it. - ReadLocks[instanceId] = new Dictionary(); - ReadLocks[instanceId][lockId] = 1; - } + locksDict.TryGetValue(lockId, out var value); + ReadLocks[instanceId][lockId] = value + 1; } else { - ParentScope.IncrementReadLock(lockId, instanceId); + // The scope hasn't requested a lock yet, so we have to create a dict for it. + ReadLocks[instanceId] = new Dictionary(); + ReadLocks[instanceId][lockId] = 1; } } + /// + /// Resets all read lock counters for a given scope instance to 0, signalling that the lock is no longer in use. + /// + /// Instance ID of the scope to clear of lock counters. private void ClearReadLocks(Guid instanceId) { if (ParentScope != null) @@ -585,7 +578,7 @@ namespace Umbraco.Core.Scoping { lock (_dictionaryLocker) { - // Rest all values to 0 since the scope has been disposed + // Reset all values to 0 since the scope is being disposed if (ReadLocks.ContainsKey(instanceId)) { foreach (var key in ReadLocks[instanceId].Keys.ToList()) @@ -597,6 +590,10 @@ namespace Umbraco.Core.Scoping } } + /// + /// Resets all write lock counters for a given scope instance to 0, signalling that the lock is no longer in use. + /// + /// Instance ID of the scope to clear of lock counters. private void ClearWriteLocks(Guid instanceID) { if (ParentScope != null) @@ -607,6 +604,7 @@ namespace Umbraco.Core.Scoping { lock (_dictionaryLocker) { + // Reset all values to 0 since the scope is being disposed if (WriteLocks.ContainsKey(instanceID)) { foreach (var key in WriteLocks[instanceID].Keys.ToList()) @@ -652,7 +650,7 @@ namespace Umbraco.Core.Scoping { // Check if there is any dictionary with a key equal to lockId // And check that the value associated with that key is greater than 0, if not it could be because a lock was requested but it failed. - return ReadLocks.Values.Any(x => x.ContainsKey(lockId)); + return ReadLocks.Values.Any(x => x.ContainsKey(lockId)); } /// @@ -705,6 +703,7 @@ namespace Umbraco.Core.Scoping { // Something went wrong and we didn't get the lock // Since we at this point have determined that we haven't got any key of LockID, it's safe to completely remove it instead of decrementing. + // It needs to be completely removed, because that's how we determine to acquire a lock. ReadLocks[instanceId].Remove(lockId); throw; } @@ -755,6 +754,7 @@ namespace Umbraco.Core.Scoping { // Something went wrong and we didn't get the lock // Since we at this point have determined that we haven't got any key of LockID, it's safe to completely remove it instead of decrementing. + // It needs to be completely removed, because that's how we determine to acquire a lock. WriteLocks[instanceId].Remove(lockId); throw; } diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index 0cc05bb5d5..789cbeff88 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -466,7 +466,7 @@ namespace Umbraco.Tests.Scoping readDict[Constants.Locks.Languages] = 1; scope.ReadLocks[Guid.NewGuid()] = readDict; - Assert.Throws(() => scope.Dispose()); + Assert.Throws(() => scope.Dispose()); // We have to clear so we can properly dispose the scope, otherwise it'll mess with other tests. scope.ReadLocks.Clear(); @@ -483,7 +483,7 @@ namespace Umbraco.Tests.Scoping writeDict[Constants.Locks.Languages] = 1; scope.WriteLocks[Guid.NewGuid()] = writeDict; - Assert.Throws(() => scope.Dispose()); + Assert.Throws(() => scope.Dispose()); // We have to clear so we can properly dispose the scope, otherwise it'll mess with other tests. scope.WriteLocks.Clear(); From 54a4a76efb37c3e1c1e2799ebd3a0807ba9b1dd4 Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Mar 2021 11:10:34 +0100 Subject: [PATCH 07/14] Use a hashset to keep track of acquired locks This simplifies disposing/checking for locks greatly. --- src/Umbraco.Core/Scoping/Scope.cs | 53 +++++++-------------- src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 24 ++++------ 2 files changed, 24 insertions(+), 53 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index f6ec36125d..880db57a2b 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -37,6 +37,8 @@ namespace Umbraco.Core.Scoping private IEventDispatcher _eventDispatcher; private object _dictionaryLocker; + private readonly HashSet _readLocks; + private readonly HashSet _writeLocks; // 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. @@ -68,8 +70,10 @@ namespace Umbraco.Core.Scoping Detachable = detachable; _dictionaryLocker = new object(); - ReadLocks = new Dictionary>(); WriteLocks = new Dictionary>(); + ReadLocks = new Dictionary>(); + _writeLocks = new HashSet(); + _readLocks = new HashSet(); #if DEBUG_SCOPES _scopeProvider.RegisterScope(this); @@ -370,8 +374,7 @@ namespace Umbraco.Core.Scoping { // We're the parent scope, make sure that locks of all scopes has been cleared // Since we're only reading we don't have to be in a lock - if (ReadLocks.Values.Any(x => x.Values.Any(value => value != 0)) - || WriteLocks.Values.Any(x => x.Values.Any(value => value != 0))) + if (ReadLocks.Any() || WriteLocks.Any()) { throw new InvalidOperationException($"All scopes has not been disposed from parent scope: {InstanceId}"); } @@ -580,13 +583,10 @@ namespace Umbraco.Core.Scoping { lock (_dictionaryLocker) { - // Reset all values to 0 since the scope is being disposed + // Remove the scope from the dictionary because it's getting disposed. if (ReadLocks.ContainsKey(instanceId)) { - foreach (var key in ReadLocks[instanceId].Keys.ToList()) - { - ReadLocks[instanceId][key] = 0; - } + ReadLocks.Remove(instanceId); } } } @@ -606,13 +606,10 @@ namespace Umbraco.Core.Scoping { lock (_dictionaryLocker) { - // Reset all values to 0 since the scope is being disposed + // Remove the scope from the dictionary because it's getting disposed. if (WriteLocks.ContainsKey(instanceID)) { - foreach (var key in WriteLocks[instanceID].Keys.ToList()) - { - WriteLocks[instanceID][key] = 0; - } + WriteLocks.Remove(instanceID); } } @@ -643,28 +640,6 @@ namespace Umbraco.Core.Scoping WriteLockInner(InstanceId, timeout, lockId); } - /// - /// Determine if a read lock with the specified ID has already been obtained. - /// - /// Id to test. - /// True if no scopes has obtained a read lock with the specific ID yet. - private bool HasReadLock(int lockId) - { - // Check if there is any dictionary with a key equal to lockId - // And check that the value associated with that key is greater than 0, if not it could be because a lock was requested but it failed. - return ReadLocks.Values.Any(x => x.ContainsKey(lockId)); - } - - /// - /// Determine if a write lock with the specified ID has already been obtained. - /// - /// Id to test - /// >True if no scopes has obtained a write lock with the specific ID yet. - private bool HasWriteLock(int lockId) - { - return WriteLocks.Values.Any(x => x.ContainsKey(lockId)); - } - /// /// Handles acquiring a read lock, will delegate it to the parent if there are any. /// @@ -685,9 +660,10 @@ namespace Umbraco.Core.Scoping foreach (var lockId in lockIds) { // Only acquire the lock if we haven't done so yet. - if (!HasReadLock(lockId)) + if (!_readLocks.Contains(lockId)) { IncrementReadLock(lockId, instanceId); + _readLocks.Add(lockId); try { if (timeout is null) @@ -707,6 +683,7 @@ namespace Umbraco.Core.Scoping // Since we at this point have determined that we haven't got any key of LockID, it's safe to completely remove it instead of decrementing. // It needs to be completely removed, because that's how we determine to acquire a lock. ReadLocks[instanceId].Remove(lockId); + _readLocks.Remove(lockId); throw; } } @@ -738,9 +715,10 @@ namespace Umbraco.Core.Scoping foreach (var lockId in lockIds) { // Only acquire lock if we haven't yet (WriteLocks not containing the key) - if (!HasWriteLock(lockId)) + if (!_writeLocks.Contains(lockId)) { IncrementWriteLock(lockId, instanceId); + _writeLocks.Add(lockId); try { if (timeout is null) @@ -758,6 +736,7 @@ namespace Umbraco.Core.Scoping // Since we at this point have determined that we haven't got any key of LockID, it's safe to completely remove it instead of decrementing. // It needs to be completely removed, because that's how we determine to acquire a lock. WriteLocks[instanceId].Remove(lockId); + _writeLocks.Remove(lockId); throw; } } diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index 789cbeff88..2a4f72f9a1 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -254,8 +254,7 @@ namespace Umbraco.Tests.Scoping innerScope.Complete(); } Assert.AreEqual(2, realOuterScope.WriteLocks[realOuterScope.InstanceId][Constants.Locks.ContentTree]); - Assert.AreEqual(0, realOuterScope.WriteLocks[innerscopeId][Constants.Locks.Languages]); - Assert.AreEqual(0, realOuterScope.WriteLocks[innerscopeId][Constants.Locks.ContentTree]); + Assert.IsFalse(realOuterScope.WriteLocks.ContainsKey(innerscopeId)); outerscope.Complete(); } } @@ -287,8 +286,7 @@ namespace Umbraco.Tests.Scoping innerScope.Complete(); } Assert.AreEqual(2, realOuterScope.ReadLocks[outerscope.InstanceId][Constants.Locks.ContentTree]); - Assert.AreEqual(0, realOuterScope.ReadLocks[innerscopeId][Constants.Locks.Languages]); - Assert.AreEqual(0, realOuterScope.ReadLocks[innerscopeId][Constants.Locks.ContentTree]); + Assert.IsFalse(realOuterScope.ReadLocks.ContainsKey(innerscopeId)); outerscope.Complete(); @@ -344,18 +342,14 @@ namespace Umbraco.Tests.Scoping Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope1, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.Languages], $"innerScope1, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.Languages)}"); - Assert.AreEqual(0, realParentScope.WriteLocks[innerScope2Id][Constants.Locks.ContentTree], $"innerScope1, innerScope2 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(0, realParentScope.WriteLocks[innerScope2Id][Constants.Locks.MediaTypes], $"innerScope1, innerScope2 instance, after innserScope2 disposed: {nameof(Constants.Locks.MediaTypes)}"); + Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope2Id)); innerScope1.Complete(); } Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(0, realParentScope.WriteLocks[innerScope2Id][Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(0, realParentScope.WriteLocks[innerScope2Id][Constants.Locks.MediaTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.MediaTypes)}"); - Assert.AreEqual(0, realParentScope.WriteLocks[innerScope1Id][Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); - Assert.AreEqual(0, realParentScope.WriteLocks[innerScope1Id][Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(0, realParentScope.WriteLocks[innerScope1Id][Constants.Locks.Languages], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.Languages)}"); + Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope2Id)); + Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope1Id)); parentScope.Complete(); } @@ -408,17 +402,15 @@ namespace Umbraco.Tests.Scoping Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope1, innerScope1 instance, after innerScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, innerScope1 instance, after innerScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); Assert.AreEqual(1, realParentScope.ReadLocks[innserScope1.InstanceId][Constants.Locks.Languages], $"innerScope1, innerScope1 instance, after innerScope2 disposed: {nameof(Constants.Locks.Languages)}"); - Assert.AreEqual(0, realParentScope.ReadLocks[innerScope2Id][Constants.Locks.ContentTree]); - Assert.AreEqual(0, realParentScope.ReadLocks[innerScope2Id][Constants.Locks.MediaTypes]); + Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope2Id)); innserScope1.Complete(); } Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"parentScope after innerScope1 disposed: {nameof(Constants.Locks.ContentTree)}"); Assert.AreEqual(1, realParentScope.ReadLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after innerScope1 disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.AreEqual(0, realParentScope.ReadLocks[innerScope1Id][Constants.Locks.ContentTree]); - Assert.AreEqual(0, realParentScope.ReadLocks[innerScope1Id][Constants.Locks.ContentTypes]); - Assert.AreEqual(0, realParentScope.ReadLocks[innerScope1Id][Constants.Locks.Languages]); + Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope2Id)); + Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope1Id)); parentScope.Complete(); } From 9bd1b06eaf4638f61795fa9dcb791f2c20670d8e Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Mar 2021 13:09:45 +0100 Subject: [PATCH 08/14] Only create the dicts and hashset when a lock is requested --- src/Umbraco.Core/Scoping/Scope.cs | 33 ++++++--- src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 74 ++++++++++++++++----- 2 files changed, 81 insertions(+), 26 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 880db57a2b..0a3051855a 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -37,13 +37,13 @@ namespace Umbraco.Core.Scoping private IEventDispatcher _eventDispatcher; private object _dictionaryLocker; - private readonly HashSet _readLocks; - private readonly HashSet _writeLocks; + private HashSet _readLocks; + private HashSet _writeLocks; // 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. - internal readonly Dictionary> ReadLocks; - internal readonly Dictionary> WriteLocks; + internal Dictionary> ReadLocks { get; private set; } + internal Dictionary> WriteLocks { get; private set; } // initializes a new scope private Scope(ScopeProvider scopeProvider, @@ -70,10 +70,6 @@ namespace Umbraco.Core.Scoping Detachable = detachable; _dictionaryLocker = new object(); - WriteLocks = new Dictionary>(); - ReadLocks = new Dictionary>(); - _writeLocks = new HashSet(); - _readLocks = new HashSet(); #if DEBUG_SCOPES _scopeProvider.RegisterScope(this); @@ -374,7 +370,7 @@ namespace Umbraco.Core.Scoping { // We're the parent scope, make sure that locks of all scopes has been cleared // Since we're only reading we don't have to be in a lock - if (ReadLocks.Any() || WriteLocks.Any()) + if (ReadLocks is not null && ReadLocks.Any() || WriteLocks is not null && WriteLocks.Any()) { throw new InvalidOperationException($"All scopes has not been disposed from parent scope: {InstanceId}"); } @@ -529,6 +525,9 @@ namespace Umbraco.Core.Scoping private void IncrementWriteLock(int lockId, Guid instanceId) { // Since we've already checked that we're the parent in the WriteLockInner method, we don't need to check again. + // If it's the very first time a lock has been requested the WriteLocks dict hasn't been instantiated yet. + WriteLocks ??= new Dictionary>(); + // Try and get the dict associated with the scope id. var locksDictFound = WriteLocks.TryGetValue(instanceId, out var locksDict); if (locksDictFound) @@ -555,6 +554,9 @@ namespace Umbraco.Core.Scoping private void IncrementReadLock(int lockId, Guid instanceId) { // Since we've already checked that we're the parent in the WriteLockInner method, we don't need to check again. + // If it's the very first time a lock has been requested the ReadLocks dict hasn't been instantiated yet. + ReadLocks ??= new Dictionary>(); + var locksDictFound = ReadLocks.TryGetValue(instanceId, out var locksDict); if (locksDictFound) { @@ -581,6 +583,12 @@ namespace Umbraco.Core.Scoping } else { + if (ReadLocks is null) + { + // If ReadLocks is null, no locks has been requested and we don't have to do anything. + return; + } + lock (_dictionaryLocker) { // Remove the scope from the dictionary because it's getting disposed. @@ -604,6 +612,11 @@ namespace Umbraco.Core.Scoping } else { + if (WriteLocks is null) + { + return; + } + lock (_dictionaryLocker) { // Remove the scope from the dictionary because it's getting disposed. @@ -656,6 +669,7 @@ namespace Umbraco.Core.Scoping lock (_dictionaryLocker) { + _readLocks ??= new HashSet(); // If we are the parent, then handle the lock request. foreach (var lockId in lockIds) { @@ -712,6 +726,7 @@ namespace Umbraco.Core.Scoping lock (_dictionaryLocker) { + _writeLocks ??= new HashSet(); foreach (var lockId in lockIds) { // Only acquire lock if we haven't yet (WriteLocks not containing the key) diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index 2a4f72f9a1..579c0cc7cf 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -342,14 +342,14 @@ namespace Umbraco.Tests.Scoping Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTree], $"innerScope1, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTree)}"); Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.ContentTypes], $"innerScope1, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.ContentTypes)}"); Assert.AreEqual(1, realParentScope.WriteLocks[innerScope1.InstanceId][Constants.Locks.Languages], $"innerScope1, innerScope1 instance, after innserScope2 disposed: {nameof(Constants.Locks.Languages)}"); - Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope2Id)); + Assert.IsFalse(realParentScope.WriteLocks.ContainsKey(innerScope2Id)); innerScope1.Complete(); } Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTree], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTree)}"); Assert.AreEqual(1, realParentScope.WriteLocks[realParentScope.InstanceId][Constants.Locks.ContentTypes], $"parentScope after inner scopes disposed: {nameof(Constants.Locks.ContentTypes)}"); - Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope2Id)); - Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope1Id)); + Assert.IsFalse(realParentScope.WriteLocks.ContainsKey(innerScope2Id)); + Assert.IsFalse(realParentScope.WriteLocks.ContainsKey(innerScope1Id)); parentScope.Complete(); } @@ -454,15 +454,23 @@ namespace Umbraco.Tests.Scoping var scopeprovider = GetScopeProvider(out var syntaxProviderMock); var scope = (Scope) scopeprovider.CreateScope(); - var readDict = new Dictionary(); - readDict[Constants.Locks.Languages] = 1; - scope.ReadLocks[Guid.NewGuid()] = readDict; + try + { + // Request a lock to create the ReadLocks dict. + scope.ReadLock(Constants.Locks.Domains); - Assert.Throws(() => scope.Dispose()); + var readDict = new Dictionary(); + readDict[Constants.Locks.Languages] = 1; + scope.ReadLocks[Guid.NewGuid()] = readDict; - // We have to clear so we can properly dispose the scope, otherwise it'll mess with other tests. - scope.ReadLocks.Clear(); - scope.Dispose(); + Assert.Throws(() => scope.Dispose()); + } + finally + { + // We have to clear so we can properly dispose the scope, otherwise it'll mess with other tests. + scope.ReadLocks?.Clear(); + scope.Dispose(); + } } [Test] @@ -471,15 +479,47 @@ namespace Umbraco.Tests.Scoping var scopeprovider = GetScopeProvider(out var syntaxProviderMock); var scope = (Scope) scopeprovider.CreateScope(); - var writeDict = new Dictionary(); - writeDict[Constants.Locks.Languages] = 1; - scope.WriteLocks[Guid.NewGuid()] = writeDict; + try + { + // Request a lock to create the ReadLocks dict. + scope.WriteLock(Constants.Locks.Domains); - Assert.Throws(() => scope.Dispose()); + var writeDict = new Dictionary(); + writeDict[Constants.Locks.Languages] = 1; + scope.WriteLocks[Guid.NewGuid()] = writeDict; - // We have to clear so we can properly dispose the scope, otherwise it'll mess with other tests. - scope.WriteLocks.Clear(); - scope.Dispose(); + Assert.Throws(() => scope.Dispose()); + } + finally + { + // We have to clear so we can properly dispose the scope, otherwise it'll mess with other tests. + scope.WriteLocks?.Clear(); + scope.Dispose(); + } + } + + [Test] + public void WriteLocks_Not_Created_Until_First_Lock() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var scope = scopeProvider.CreateScope()) + { + var realScope = (Scope) scope; + Assert.IsNull(realScope.WriteLocks); + } + } + + [Test] + public void ReadLocks_Not_Created_Until_First_Lock() + { + var scopeProvider = GetScopeProvider(out var syntaxProviderMock); + + using (var scope = scopeProvider.CreateScope()) + { + var realScope = (Scope) scope; + Assert.IsNull(realScope.ReadLocks); + } } } } From c504e3ab7e77ea720611cf04b5cae176269a301a Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 17 Mar 2021 13:40:14 +0100 Subject: [PATCH 09/14] Clean --- src/Umbraco.Core/Scoping/Scope.cs | 55 ++++++++++--------------------- 1 file changed, 18 insertions(+), 37 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 0a3051855a..3729bf333e 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -39,9 +39,6 @@ namespace Umbraco.Core.Scoping private object _dictionaryLocker; private HashSet _readLocks; private HashSet _writeLocks; - - // 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. internal Dictionary> ReadLocks { get; private set; } internal Dictionary> WriteLocks { get; private set; } @@ -577,25 +574,16 @@ namespace Umbraco.Core.Scoping /// Instance ID of the scope to clear of lock counters. private void ClearReadLocks(Guid instanceId) { - if (ParentScope != null) + if (ParentScope is not null) { ParentScope.ClearReadLocks(instanceId); } else { - if (ReadLocks is null) - { - // If ReadLocks is null, no locks has been requested and we don't have to do anything. - return; - } - lock (_dictionaryLocker) { // Remove the scope from the dictionary because it's getting disposed. - if (ReadLocks.ContainsKey(instanceId)) - { - ReadLocks.Remove(instanceId); - } + ReadLocks?.Remove(instanceId); } } } @@ -606,26 +594,17 @@ namespace Umbraco.Core.Scoping /// Instance ID of the scope to clear of lock counters. private void ClearWriteLocks(Guid instanceID) { - if (ParentScope != null) + if (ParentScope is not null) { ParentScope.ClearWriteLocks(instanceID); } else { - if (WriteLocks is null) - { - return; - } - lock (_dictionaryLocker) { // Remove the scope from the dictionary because it's getting disposed. - if (WriteLocks.ContainsKey(instanceID)) - { - WriteLocks.Remove(instanceID); - } + WriteLocks?.Remove(instanceID); } - } } @@ -656,11 +635,12 @@ namespace Umbraco.Core.Scoping /// /// Handles acquiring a read lock, will delegate it to the parent if there are any. /// + /// Instance ID of the requesting scope. /// Optional database timeout in milliseconds. /// Array of lock object identifiers. private void ReadLockInner(Guid instanceId, TimeSpan? timeout = null, params int[] lockIds) { - if (ParentScope != null) + if (ParentScope is not null) { // Delegate acquiring the lock to the parent if any. ParentScope.ReadLockInner(instanceId, timeout, lockIds); @@ -670,7 +650,7 @@ namespace Umbraco.Core.Scoping lock (_dictionaryLocker) { _readLocks ??= new HashSet(); - // If we are the parent, then handle the lock request. + // We are the parent handle the lock request. foreach (var lockId in lockIds) { // Only acquire the lock if we haven't done so yet. @@ -694,16 +674,16 @@ namespace Umbraco.Core.Scoping catch { // Something went wrong and we didn't get the lock - // Since we at this point have determined that we haven't got any key of LockID, it's safe to completely remove it instead of decrementing. - // It needs to be completely removed, because that's how we determine to acquire a lock. + // Since we at this point have determined that we haven't got any lock with an ID of LockID, it's safe to completely remove it instead of decrementing. ReadLocks[instanceId].Remove(lockId); + // It needs to be removed from the HashSet as well, because that's how we determine to acquire a lock. _readLocks.Remove(lockId); throw; } } else { - // We already have a lock, but need to update the readlock dictionary for debugging purposes. + // We already have a lock, but need to update the ReadLock dictionary for debugging purposes. IncrementReadLock(lockId, instanceId); } } @@ -713,11 +693,12 @@ namespace Umbraco.Core.Scoping /// /// Handles acquiring a write lock with a specified timeout, will delegate it to the parent if there are any. /// + /// Instance ID of the requesting scope. /// Optional database timeout in milliseconds. /// Array of lock object identifiers. - internal void WriteLockInner(Guid instanceId, TimeSpan? timeout = null, params int[] lockIds) + private void WriteLockInner(Guid instanceId, TimeSpan? timeout = null, params int[] lockIds) { - if (ParentScope != null) + if (ParentScope is not null) { // If we have a parent we delegate lock creation to parent. ParentScope.WriteLockInner(instanceId, timeout, lockIds); @@ -729,7 +710,7 @@ namespace Umbraco.Core.Scoping _writeLocks ??= new HashSet(); foreach (var lockId in lockIds) { - // Only acquire lock if we haven't yet (WriteLocks not containing the key) + // Only acquire lock if we haven't yet if (!_writeLocks.Contains(lockId)) { IncrementWriteLock(lockId, instanceId); @@ -748,9 +729,9 @@ namespace Umbraco.Core.Scoping catch { // Something went wrong and we didn't get the lock - // Since we at this point have determined that we haven't got any key of LockID, it's safe to completely remove it instead of decrementing. - // It needs to be completely removed, because that's how we determine to acquire a lock. + // Since we at this point have determined that we haven't got any lock with an ID of LockID, it's safe to completely remove it instead of decrementing. WriteLocks[instanceId].Remove(lockId); + // It needs to be removed from the HashSet as well, because that's how we determine to acquire a lock. _writeLocks.Remove(lockId); throw; } @@ -781,7 +762,7 @@ namespace Umbraco.Core.Scoping private void ObtainTimoutReadLock(int lockId, TimeSpan timeout) { var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; - if (syntax2 == null) + if (syntax2 is null) { throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); } @@ -806,7 +787,7 @@ namespace Umbraco.Core.Scoping private void ObtainTimeoutWriteLock(int lockId, TimeSpan timeout) { var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; - if (syntax2 == null) + if (syntax2 is null) { throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); } From 8f50bdbef48751649edac7c0c8cf314e0e80d1d4 Mon Sep 17 00:00:00 2001 From: Mole Date: Thu, 18 Mar 2021 11:24:14 +0100 Subject: [PATCH 10/14] Apply suggestions from review --- src/Umbraco.Core/Scoping/Scope.cs | 231 ++++++++------------ src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 3 +- 2 files changed, 94 insertions(+), 140 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 3729bf333e..04e1934bcc 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Data; using System.Linq; +using System.Text; using Umbraco.Core.Cache; using Umbraco.Core.Composing; using Umbraco.Core.Events; @@ -39,8 +40,8 @@ namespace Umbraco.Core.Scoping private object _dictionaryLocker; private HashSet _readLocks; private HashSet _writeLocks; - internal Dictionary> ReadLocks { get; private set; } - internal Dictionary> WriteLocks { get; private set; } + internal Dictionary> ReadLocks; + internal Dictionary> WriteLocks; // initializes a new scope private Scope(ScopeProvider scopeProvider, @@ -361,15 +362,45 @@ namespace Umbraco.Core.Scoping } // Decrement the lock counters on the parent if any. - ClearReadLocks(InstanceId); - ClearWriteLocks(InstanceId); + ClearLocks(InstanceId); if (ParentScope is null) { // We're the parent scope, make sure that locks of all scopes has been cleared // Since we're only reading we don't have to be in a lock if (ReadLocks is not null && ReadLocks.Any() || WriteLocks is not null && WriteLocks.Any()) { - throw new InvalidOperationException($"All scopes has not been disposed from parent scope: {InstanceId}"); + // Dump the dicts into a message for the locks. + StringBuilder builder = new StringBuilder(); + builder.AppendLine($"Lock counters aren't empty, suggesting a scope hasn't been properly disposed, parent id: {InstanceId}"); + if (ReadLocks is not null && ReadLocks.Any()) + { + builder.AppendLine("Remaining ReadLocks:"); + foreach (var locksValues in ReadLocks) + { + builder.AppendLine($"Scope {locksValues.Key}:"); + foreach (var lockCounter in locksValues.Value) + { + builder.AppendLine($"\tLock ID: {lockCounter.Key} - times requested: {lockCounter.Value}"); + } + } + } + + if (WriteLocks is not null && WriteLocks.Any()) + { + builder.AppendLine("Remaining WriteLocks:"); + foreach (var locksValues in WriteLocks) + { + builder.AppendLine($"Scope {locksValues.Key}:"); + foreach (var lockCounter in locksValues.Value) + { + builder.AppendLine($"\tLock ID: {lockCounter.Key}, amount requested: {lockCounter.Value}"); + } + } + } + + var exception = new InvalidOperationException($"All scopes has not been disposed from parent scope: {InstanceId}"); + _logger.Error(exception, builder.ToString()); + throw exception; } } @@ -512,125 +543,64 @@ namespace Umbraco.Core.Scoping ?? (_logUncompletedScopes = Current.Configs.CoreDebug().LogUncompletedScopes)).Value; /// - /// Increment the counter in WriteLocks for a specific scope instance and lock identifier. + /// Increment the counter of a locks dictionary, either ReadLocks or WriteLocks, + /// for a specific scope instance and lock identifier. Must be called within a lock. /// - /// - /// This will not lock on the lock object, ensure that you lock on _dictionaryLocker before calling this method. - /// /// Lock ID to increment. /// Instance ID of the scope requesting the lock. - private void IncrementWriteLock(int lockId, Guid instanceId) + /// Reference to the dictionary to increment on + private void IncrementLock(int lockId, Guid instanceId, ref Dictionary> locks) { // Since we've already checked that we're the parent in the WriteLockInner method, we don't need to check again. // If it's the very first time a lock has been requested the WriteLocks dict hasn't been instantiated yet. - WriteLocks ??= new Dictionary>(); + locks ??= new Dictionary>(); // Try and get the dict associated with the scope id. - var locksDictFound = WriteLocks.TryGetValue(instanceId, out var locksDict); + var locksDictFound = locks.TryGetValue(instanceId, out var locksDict); if (locksDictFound) { locksDict.TryGetValue(lockId, out var value); - WriteLocks[instanceId][lockId] = value + 1; + locksDict[lockId] = value + 1; } else { // The scope hasn't requested a lock yet, so we have to create a dict for it. - WriteLocks[instanceId] = new Dictionary(); - WriteLocks[instanceId][lockId] = 1; + locks.Add(instanceId, new Dictionary()); + locks[instanceId][lockId] = 1; } } /// - /// Increment the counter in ReadLocks for a specific scope instance and lock identifier. + /// Clears all lock counters for a given scope instance, signalling that the scope has been disposed. /// - /// - /// This will not lock on the lock object, ensure that you lock on _dictionaryLocker before calling this method. - /// - /// Lock ID to increment. - /// Instance ID of the scope requesting the lock. - private void IncrementReadLock(int lockId, Guid instanceId) - { - // Since we've already checked that we're the parent in the WriteLockInner method, we don't need to check again. - // If it's the very first time a lock has been requested the ReadLocks dict hasn't been instantiated yet. - ReadLocks ??= new Dictionary>(); - - var locksDictFound = ReadLocks.TryGetValue(instanceId, out var locksDict); - if (locksDictFound) - { - locksDict.TryGetValue(lockId, out var value); - ReadLocks[instanceId][lockId] = value + 1; - } - else - { - // The scope hasn't requested a lock yet, so we have to create a dict for it. - ReadLocks[instanceId] = new Dictionary(); - ReadLocks[instanceId][lockId] = 1; - } - } - - /// - /// Resets all read lock counters for a given scope instance to 0, signalling that the lock is no longer in use. - /// - /// Instance ID of the scope to clear of lock counters. - private void ClearReadLocks(Guid instanceId) + /// Instance ID of the scope to clear. + private void ClearLocks(Guid instanceId) { if (ParentScope is not null) { - ParentScope.ClearReadLocks(instanceId); + ParentScope.ClearLocks(instanceId); } else { lock (_dictionaryLocker) { - // Remove the scope from the dictionary because it's getting disposed. ReadLocks?.Remove(instanceId); - } - } - } - - /// - /// Resets all write lock counters for a given scope instance to 0, signalling that the lock is no longer in use. - /// - /// Instance ID of the scope to clear of lock counters. - private void ClearWriteLocks(Guid instanceID) - { - if (ParentScope is not null) - { - ParentScope.ClearWriteLocks(instanceID); - } - else - { - lock (_dictionaryLocker) - { - // Remove the scope from the dictionary because it's getting disposed. - WriteLocks?.Remove(instanceID); + WriteLocks?.Remove(instanceId); } } } /// - public void ReadLock(params int[] lockIds) - { - ReadLockInner(InstanceId, null, lockIds); - } + public void ReadLock(params int[] lockIds) => ReadLockInner(InstanceId, null, lockIds); /// - public void ReadLock(TimeSpan timeout, int lockId) - { - ReadLockInner(InstanceId, timeout, lockId); - } + public void ReadLock(TimeSpan timeout, int lockId) => ReadLockInner(InstanceId, timeout, lockId); /// - public void WriteLock(params int[] lockIds) - { - WriteLockInner(InstanceId, null, lockIds); - } + public void WriteLock(params int[] lockIds) => WriteLockInner(InstanceId, null, lockIds); /// - public void WriteLock(TimeSpan timeout, int lockId) - { - WriteLockInner(InstanceId, timeout, lockId); - } + public void WriteLock(TimeSpan timeout, int lockId) => WriteLockInner(InstanceId, timeout, lockId); /// /// Handles acquiring a read lock, will delegate it to the parent if there are any. @@ -642,51 +612,13 @@ namespace Umbraco.Core.Scoping { if (ParentScope is not null) { - // Delegate acquiring the lock to the parent if any. + // If we have a parent we delegate lock creation to parent. ParentScope.ReadLockInner(instanceId, timeout, lockIds); - return; } - - lock (_dictionaryLocker) + else { - _readLocks ??= new HashSet(); - // We are the parent handle the lock request. - foreach (var lockId in lockIds) - { - // Only acquire the lock if we haven't done so yet. - if (!_readLocks.Contains(lockId)) - { - IncrementReadLock(lockId, instanceId); - _readLocks.Add(lockId); - try - { - if (timeout is null) - { - // We just want an ordinary lock. - ObtainReadLock(lockId); - } - else - { - // We want a lock with a custom timeout - ObtainTimoutReadLock(lockId, timeout.Value); - } - } - catch - { - // Something went wrong and we didn't get the lock - // Since we at this point have determined that we haven't got any lock with an ID of LockID, it's safe to completely remove it instead of decrementing. - ReadLocks[instanceId].Remove(lockId); - // It needs to be removed from the HashSet as well, because that's how we determine to acquire a lock. - _readLocks.Remove(lockId); - throw; - } - } - else - { - // We already have a lock, but need to update the ReadLock dictionary for debugging purposes. - IncrementReadLock(lockId, instanceId); - } - } + // We are the outermost scope, handle the lock request. + LockInner(instanceId, ref ReadLocks, ref _readLocks, ObtainReadLock, ObtainTimeoutReadLock, timeout, lockIds); } } @@ -702,44 +634,65 @@ namespace Umbraco.Core.Scoping { // If we have a parent we delegate lock creation to parent. ParentScope.WriteLockInner(instanceId, timeout, lockIds); - return; } + else + { + // We are the outermost scope, handle the lock request. + LockInner(instanceId, ref WriteLocks, ref _writeLocks, ObtainWriteLock, ObtainTimeoutWriteLock, timeout, lockIds); + } + } + /// + /// Handles acquiring a lock, this should only be called from the outermost scope. + /// + /// Instance ID of the scope requesting the lock. + /// Reference to the applicable locks dictionary (ReadLocks or WriteLocks). + /// Reference to the applicable locks hashset (_readLocks or _writeLocks). + /// Delegate used to request the lock from the database without a timeout. + /// Delegate used to request the lock from the database with a timeout. + /// Optional timeout parameter to specify a timeout. + /// Lock identifiers to lock on. + private void LockInner(Guid instanceId, ref Dictionary> locks, ref HashSet locksSet, + Action obtainLock, Action obtainLockTimeout, TimeSpan? timeout = null, + params int[] lockIds) + { lock (_dictionaryLocker) { - _writeLocks ??= new HashSet(); + locksSet ??= new HashSet(); foreach (var lockId in lockIds) { - // Only acquire lock if we haven't yet - if (!_writeLocks.Contains(lockId)) + // Only acquire the lock if we haven't done so yet. + if (!locksSet.Contains(lockId)) { - IncrementWriteLock(lockId, instanceId); - _writeLocks.Add(lockId); + IncrementLock(lockId, instanceId, ref locks); + locksSet.Add(lockId); try { if (timeout is null) { - ObtainWriteLock(lockId); + // We just want an ordinary lock. + obtainLock(lockId); } else { - ObtainTimeoutWriteLock(lockId, timeout.Value); + // We want a lock with a custom timeout + obtainLockTimeout(lockId, timeout.Value); } } catch { // Something went wrong and we didn't get the lock // Since we at this point have determined that we haven't got any lock with an ID of LockID, it's safe to completely remove it instead of decrementing. - WriteLocks[instanceId].Remove(lockId); + locks[instanceId].Remove(lockId); // It needs to be removed from the HashSet as well, because that's how we determine to acquire a lock. - _writeLocks.Remove(lockId); + locksSet.Remove(lockId); throw; } } else { - // We already have a lock, so just increment the count for debugging purposes - IncrementWriteLock(lockId, instanceId); + // We already have a lock, but need to update the dictionary for debugging purposes. + IncrementLock(lockId, instanceId, ref locks); } } } @@ -759,7 +712,7 @@ namespace Umbraco.Core.Scoping /// /// Lock object identifier to lock. /// TimeSpan specifying the timout period. - private void ObtainTimoutReadLock(int lockId, TimeSpan timeout) + private void ObtainTimeoutReadLock(int lockId, TimeSpan timeout) { var syntax2 = Database.SqlContext.SqlSyntax as ISqlSyntaxProvider2; if (syntax2 is null) diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index 579c0cc7cf..cb0a1c1bea 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -10,6 +10,7 @@ using Umbraco.Core.Logging; using Umbraco.Core.Persistence; using Umbraco.Core.Persistence.SqlSyntax; using Umbraco.Core.Scoping; +using Umbraco.Tests.TestHelpers; namespace Umbraco.Tests.Scoping { @@ -481,7 +482,7 @@ namespace Umbraco.Tests.Scoping try { - // Request a lock to create the ReadLocks dict. + // Request a lock to create the WriteLocks dict. scope.WriteLock(Constants.Locks.Domains); var writeDict = new Dictionary(); From 95f42487d4f4675cfebadc37d468b3f4b5496047 Mon Sep 17 00:00:00 2001 From: Mole Date: Fri, 19 Mar 2021 08:37:23 +0100 Subject: [PATCH 11/14] Wrap dumping dictionaries in a method. --- src/Umbraco.Core/Scoping/Scope.cs | 54 +++++++++++++++---------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 04e1934bcc..8075d9b165 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Generic; using System.Data; -using System.Linq; using System.Text; using Umbraco.Core.Cache; using Umbraco.Core.Composing; @@ -367,38 +366,15 @@ namespace Umbraco.Core.Scoping { // We're the parent scope, make sure that locks of all scopes has been cleared // Since we're only reading we don't have to be in a lock - if (ReadLocks is not null && ReadLocks.Any() || WriteLocks is not null && WriteLocks.Any()) + if (ReadLocks?.Count > 0 || WriteLocks?.Count > 0) { // Dump the dicts into a message for the locks. StringBuilder builder = new StringBuilder(); builder.AppendLine($"Lock counters aren't empty, suggesting a scope hasn't been properly disposed, parent id: {InstanceId}"); - if (ReadLocks is not null && ReadLocks.Any()) - { - builder.AppendLine("Remaining ReadLocks:"); - foreach (var locksValues in ReadLocks) - { - builder.AppendLine($"Scope {locksValues.Key}:"); - foreach (var lockCounter in locksValues.Value) - { - builder.AppendLine($"\tLock ID: {lockCounter.Key} - times requested: {lockCounter.Value}"); - } - } - } + WriteLockDictionaryToString(ReadLocks, builder, "read locks"); + WriteLockDictionaryToString(WriteLocks, builder, "write locks"); - if (WriteLocks is not null && WriteLocks.Any()) - { - builder.AppendLine("Remaining WriteLocks:"); - foreach (var locksValues in WriteLocks) - { - builder.AppendLine($"Scope {locksValues.Key}:"); - foreach (var lockCounter in locksValues.Value) - { - builder.AppendLine($"\tLock ID: {lockCounter.Key}, amount requested: {lockCounter.Value}"); - } - } - } - - var exception = new InvalidOperationException($"All scopes has not been disposed from parent scope: {InstanceId}"); + var exception = new InvalidOperationException($"All scopes has not been disposed from parent scope: {InstanceId}, see log for more details."); _logger.Error(exception, builder.ToString()); throw exception; } @@ -423,6 +399,28 @@ namespace Umbraco.Core.Scoping GC.SuppressFinalize(this); } + /// + /// Writes a locks dictionary to a for logging purposes. + /// + /// Lock dictionary to report on. + /// String builder to write to. + /// The name to report the dictionary as. + private void WriteLockDictionaryToString(Dictionary> dict, StringBuilder builder, string dictName) + { + if (dict?.Count > 0) + { + builder.AppendLine($"Remaining {dictName}:"); + foreach (var instance in dict) + { + builder.AppendLine($"Scope {instance.Key}"); + foreach (var lockCounter in instance.Value) + { + builder.AppendLine($"\tLock ID: {lockCounter.Key} - times requested: {lockCounter.Value}"); + } + } + } + } + private void DisposeLastScope() { // figure out completed From 507c821f66877f521ffc52ff079d9c7eb48f54da Mon Sep 17 00:00:00 2001 From: Mole Date: Fri, 19 Mar 2021 13:19:39 +0100 Subject: [PATCH 12/14] Create method for generating log message And remove forgotten comments. --- src/Umbraco.Core/Scoping/Scope.cs | 22 ++++++++++++++------- src/Umbraco.Tests/Scoping/ScopeUnitTests.cs | 2 -- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 8075d9b165..3a11e0661e 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -368,14 +368,8 @@ namespace Umbraco.Core.Scoping // Since we're only reading we don't have to be in a lock if (ReadLocks?.Count > 0 || WriteLocks?.Count > 0) { - // Dump the dicts into a message for the locks. - StringBuilder builder = new StringBuilder(); - builder.AppendLine($"Lock counters aren't empty, suggesting a scope hasn't been properly disposed, parent id: {InstanceId}"); - WriteLockDictionaryToString(ReadLocks, builder, "read locks"); - WriteLockDictionaryToString(WriteLocks, builder, "write locks"); - var exception = new InvalidOperationException($"All scopes has not been disposed from parent scope: {InstanceId}, see log for more details."); - _logger.Error(exception, builder.ToString()); + _logger.Error(exception, GenerateUnclearedScopesLogMessage()); throw exception; } } @@ -399,6 +393,20 @@ namespace Umbraco.Core.Scoping GC.SuppressFinalize(this); } + /// + /// Generates a log message with all scopes that hasn't cleared their locks, including how many, and what locks they have requested. + /// + /// Log message. + private string GenerateUnclearedScopesLogMessage() + { + // Dump the dicts into a message for the locks. + StringBuilder builder = new StringBuilder(); + builder.AppendLine($"Lock counters aren't empty, suggesting a scope hasn't been properly disposed, parent id: {InstanceId}"); + WriteLockDictionaryToString(ReadLocks, builder, "read locks"); + WriteLockDictionaryToString(WriteLocks, builder, "write locks"); + return builder.ToString(); + } + /// /// Writes a locks dictionary to a for logging purposes. /// diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index cb0a1c1bea..038376f71c 100644 --- a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs @@ -94,7 +94,6 @@ namespace Umbraco.Tests.Scoping outerScope.Complete(); } - // Since we request the ReadLock after the innerScope has been dispose, the key has been removed from the dictionary, and we fetch it again. syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.Languages), Times.Once); syntaxProviderMock.Verify(x => x.WriteLock(It.IsAny(), Constants.Locks.ContentTree), Times.Once); } @@ -223,7 +222,6 @@ namespace Umbraco.Tests.Scoping outerScope.Complete(); } - // Since we request the ReadLock after the innerScope has been dispose, the key has been removed from the dictionary, and we fetch it again. syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), Constants.Locks.Languages), Times.Once); syntaxProviderMock.Verify(x => x.ReadLock(It.IsAny(), Constants.Locks.ContentTree), Times.Once); } From 14c2bb4aa2a22ddc4d8a35e8eb61b7592c222fce Mon Sep 17 00:00:00 2001 From: Mole Date: Tue, 23 Mar 2021 13:03:01 +0100 Subject: [PATCH 13/14] Update cypress and fix tests --- .../cypress/integration/Settings/templates.ts | 8 ++++++-- .../cypress/integration/Tour/backofficeTour.ts | 2 +- src/Umbraco.Tests.AcceptanceTest/package.json | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Settings/templates.ts b/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Settings/templates.ts index c586384af7..65d03e5a78 100644 --- a/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Settings/templates.ts +++ b/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Settings/templates.ts @@ -23,14 +23,18 @@ context('Templates', () => { cy.umbracoEnsureTemplateNameNotExists(name); createTemplate(); + // We have to wait for the ace editor to load, because when the editor is loading it will "steal" the focus briefly, + // which causes the save event to fire if we've added something to the header field, causing errors. + cy.wait(500); + //Type name cy.umbracoEditorHeaderName(name); // Save // We must drop focus for the auto save event to occur. cy.get('.btn-success').focus(); // And then wait for the auto save event to finish by finding the page in the tree view. - // This is a bit of a roundabout way to find items in a treev view since we dont use umbracoTreeItem - // but we must be able to wait for the save evnent to finish, and we can't do that with umbracoTreeItem + // This is a bit of a roundabout way to find items in a tree view since we dont use umbracoTreeItem + // but we must be able to wait for the save event to finish, and we can't do that with umbracoTreeItem cy.get('[data-element="tree-item-templates"] > :nth-child(2) > .umb-animated > .umb-tree-item__inner > .umb-tree-item__label') .contains(name).should('be.visible', { timeout: 10000 }); // Now that the auto save event has finished we can save diff --git a/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Tour/backofficeTour.ts b/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Tour/backofficeTour.ts index 9bc1fff488..d3950d7d19 100644 --- a/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Tour/backofficeTour.ts +++ b/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Tour/backofficeTour.ts @@ -49,7 +49,7 @@ function resetTourData() { { "alias": "umbIntroIntroduction", "completed": false, - "disabled": false + "disabled": true }; cy.getCookie('UMB-XSRF-TOKEN', { log: false }).then((token) => { diff --git a/src/Umbraco.Tests.AcceptanceTest/package.json b/src/Umbraco.Tests.AcceptanceTest/package.json index 378fe719fc..069a1d85b7 100644 --- a/src/Umbraco.Tests.AcceptanceTest/package.json +++ b/src/Umbraco.Tests.AcceptanceTest/package.json @@ -7,7 +7,7 @@ }, "devDependencies": { "cross-env": "^7.0.2", - "cypress": "^6.0.1", + "cypress": "^6.8.0", "ncp": "^2.0.0", "prompt": "^1.0.0", "umbraco-cypress-testhelpers": "^1.0.0-beta-52" From f0b39ce890e8f31523739a68d317b24df79fdb76 Mon Sep 17 00:00:00 2001 From: Mole Date: Wed, 24 Mar 2021 10:57:02 +0100 Subject: [PATCH 14/14] Align 'Add language' test to netcore --- .../cypress/integration/Settings/languages.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Settings/languages.ts b/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Settings/languages.ts index 49bcf94943..336e5793d9 100644 --- a/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Settings/languages.ts +++ b/src/Umbraco.Tests.AcceptanceTest/cypress/integration/Settings/languages.ts @@ -6,7 +6,10 @@ context('Languages', () => { }); it('Add language', () => { - const name = "Kyrgyz (Kyrgyzstan)"; // Must be an option in the select box + // For some reason the languages to chose fom seems to be translated differently than normal, as an example: + // My system is set to EN (US), but most languages are translated into Danish for some reason + // Aghem seems untranslated though? + const name = "Aghem"; // Must be an option in the select box cy.umbracoEnsureLanguageNameNotExists(name);