From cee33e863ef29fc81c69bf18f9e75421beac87d3 Mon Sep 17 00:00:00 2001 From: Mole Date: Mon, 15 Mar 2021 08:38:23 +0100 Subject: [PATCH] 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(); } }