diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index af2d634e03..3a11e0661e 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.Text; using Umbraco.Core.Cache; using Umbraco.Core.Composing; using Umbraco.Core.Events; @@ -36,11 +37,10 @@ namespace Umbraco.Core.Scoping private IEventDispatcher _eventDispatcher; private object _dictionaryLocker; - - // ReadLocks and WriteLocks if we're the outer most scope it's those owned by the entire chain - // If we're a child scope it's those that we have requested. - internal readonly Dictionary ReadLocks; - internal readonly Dictionary WriteLocks; + private HashSet _readLocks; + private HashSet _writeLocks; + internal Dictionary> ReadLocks; + internal Dictionary> WriteLocks; // initializes a new scope private Scope(ScopeProvider scopeProvider, @@ -67,8 +67,6 @@ namespace Umbraco.Core.Scoping Detachable = detachable; _dictionaryLocker = new object(); - ReadLocks = new Dictionary(); - WriteLocks = new Dictionary(); #if DEBUG_SCOPES _scopeProvider.RegisterScope(this); @@ -363,19 +361,16 @@ namespace Umbraco.Core.Scoping } // Decrement the lock counters on the parent if any. - if (ParentScope != null) + ClearLocks(InstanceId); + if (ParentScope is null) { - lock (_dictionaryLocker) + // 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) { - foreach (var readLockPair in ReadLocks) - { - DecrementReadLock(readLockPair.Key, readLockPair.Value); - } - - foreach (var writeLockPair in WriteLocks) - { - DecrementWriteLock(writeLockPair.Key, writeLockPair.Value); - } + var exception = new InvalidOperationException($"All scopes has not been disposed from parent scope: {InstanceId}, see log for more details."); + _logger.Error(exception, GenerateUnclearedScopesLogMessage()); + throw exception; } } @@ -398,6 +393,42 @@ 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. + /// + /// 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 @@ -518,207 +549,157 @@ namespace Umbraco.Core.Scoping ?? (_logUncompletedScopes = Current.Configs.CoreDebug().LogUncompletedScopes)).Value; /// - /// Decrements the count of the ReadLocks with a specific lock object identifier we currently hold + /// 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. /// - /// Lock object identifier to decrement - /// Amount to decrement the lock count with - public void DecrementReadLock(int lockId, int amountToDecrement) + /// Lock ID to increment. + /// Instance ID of the scope requesting the lock. + /// Reference to the dictionary to increment on + private void IncrementLock(int lockId, Guid instanceId, ref Dictionary> locks) { - // If we aren't the outermost scope, pass it on to the parent. - if (ParentScope != null) - { - ParentScope.DecrementReadLock(lockId, amountToDecrement); - return; - } + // 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. + locks ??= new Dictionary>(); - lock (_dictionaryLocker) + // Try and get the dict associated with the scope id. + var locksDictFound = locks.TryGetValue(instanceId, out var locksDict); + if (locksDictFound) { - ReadLocks[lockId] -= amountToDecrement; + locksDict.TryGetValue(lockId, out var value); + locksDict[lockId] = value + 1; + } + else + { + // The scope hasn't requested a lock yet, so we have to create a dict for it. + locks.Add(instanceId, new Dictionary()); + locks[instanceId][lockId] = 1; } } /// - /// Decrements the count of the WriteLocks with a specific lock object identifier we currently hold. + /// Clears all lock counters for a given scope instance, signalling that the scope has been disposed. /// - /// Lock object identifier to decrement. - /// Amount to decrement the lock count with - public void DecrementWriteLock(int lockId, int amountToDecrement) + /// Instance ID of the scope to clear. + private void ClearLocks(Guid instanceId) { - // If we aren't the outermost scope, pass it on to the parent. - if (ParentScope != null) + if (ParentScope is not null) { - ParentScope.DecrementWriteLock(lockId, amountToDecrement); - return; + ParentScope.ClearLocks(instanceId); } - - lock (_dictionaryLocker) + else { - WriteLocks[lockId] -= amountToDecrement; - } - } - - /// - /// Increment the count of the read locks we've requested - /// - /// - /// This should only be done on child scopes since it's then used to decrement the count later. - /// - /// - private void IncrementRequestedReadLock(params int[] lockIds) - { - // We need to keep track of what lockIds we have requested locks for to be able to decrement them. - if (ParentScope != null) - { - foreach (var lockId in lockIds) + lock (_dictionaryLocker) { - lock (_dictionaryLocker) - { - if (ReadLocks.ContainsKey(lockId)) - { - ReadLocks[lockId] += 1; - } - else - { - ReadLocks[lockId] = 1; - } - } - } - } - } - - /// - /// Increment the count of the write locks we've requested - /// - /// - /// This should only be done on child scopes since it's then used to decrement the count later. - /// - /// - private void IncrementRequestedWriteLock(params int[] lockIds) - { - // We need to keep track of what lockIds we have requested locks for to be able to decrement them. - if (ParentScope != null) - { - foreach (var lockId in lockIds) - { - lock (_dictionaryLocker) - { - if (WriteLocks.ContainsKey(lockId)) - { - WriteLocks[lockId] += 1; - } - else - { - WriteLocks[lockId] = 1; - } - } + ReadLocks?.Remove(instanceId); + WriteLocks?.Remove(instanceId); } } } /// - public void ReadLock(params int[] lockIds) - { - IncrementRequestedReadLock(lockIds); - ReadLockInner(null, lockIds); - } + public void ReadLock(params int[] lockIds) => ReadLockInner(InstanceId, null, lockIds); /// - public void ReadLock(TimeSpan timeout, int lockId) - { - IncrementRequestedReadLock(lockId); - ReadLockInner(timeout, lockId); - } + public void ReadLock(TimeSpan timeout, int lockId) => ReadLockInner(InstanceId, timeout, lockId); /// - public void WriteLock(params int[] lockIds) - { - IncrementRequestedWriteLock(lockIds); - WriteLockInner(null, lockIds); - } + public void WriteLock(params int[] lockIds) => WriteLockInner(InstanceId, null, lockIds); /// - public void WriteLock(TimeSpan timeout, int lockId) - { - IncrementRequestedWriteLock(lockId); - WriteLockInner(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. /// + /// Instance ID of the requesting scope. /// 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) + if (ParentScope is not null) { - // Delegate acquiring the lock to the parent if any. - ParentScope.ReadLockInner(timeout, lockIds); - return; + // If we have a parent we delegate lock creation to parent. + ParentScope.ReadLockInner(instanceId, timeout, lockIds); } - - // If we are the parent, then handle the lock request. - foreach (var lockId in lockIds) + else { - lock (_dictionaryLocker) - { - // Only acquire the lock if we haven't done so yet. - if (!ReadLocks.ContainsKey(lockId)) - { - if (timeout is null) - { - // We want a lock with a custom timeout - ObtainReadLock(lockId); - } - else - { - // We just want an ordinary lock. - ObtainTimoutReadLock(lockId, timeout.Value); - } - // Add the lockId as a key to the dict. - ReadLocks[lockId] = 0; - } - - ReadLocks[lockId] += 1; - } + // We are the outermost scope, handle the lock request. + LockInner(instanceId, ref ReadLocks, ref _readLocks, ObtainReadLock, ObtainTimeoutReadLock, timeout, lockIds); } } /// /// 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(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(timeout, lockIds); - return; + ParentScope.WriteLockInner(instanceId, timeout, lockIds); } - - foreach (var lockId in lockIds) + else { - lock (_dictionaryLocker) - { - // Only acquire lock if we haven't yet (WriteLocks not containing the key) - if (!WriteLocks.ContainsKey(lockId)) - { - if (timeout is null) - { - ObtainWriteLock(lockId); - } - else - { - ObtainTimeoutWriteLock(lockId, timeout.Value); - } - // Add the lockId as a key to the dict. - WriteLocks[lockId] = 0; - } + // We are the outermost scope, handle the lock request. + LockInner(instanceId, ref WriteLocks, ref _writeLocks, ObtainWriteLock, ObtainTimeoutWriteLock, timeout, lockIds); + } + } - // Increment count of the lock by 1. - WriteLocks[lockId] += 1; + /// + /// 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) + { + locksSet ??= new HashSet(); + foreach (var lockId in lockIds) + { + // Only acquire the lock if we haven't done so yet. + if (!locksSet.Contains(lockId)) + { + IncrementLock(lockId, instanceId, ref locks); + locksSet.Add(lockId); + try + { + if (timeout is null) + { + // We just want an ordinary lock. + obtainLock(lockId); + } + else + { + // 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. + locks[instanceId].Remove(lockId); + // It needs to be removed from the HashSet as well, because that's how we determine to acquire a lock. + locksSet.Remove(lockId); + throw; + } + } + else + { + // We already have a lock, but need to update the dictionary for debugging purposes. + IncrementLock(lockId, instanceId, ref locks); + } } } } @@ -737,10 +718,10 @@ 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 == null) + if (syntax2 is null) { throw new InvalidOperationException($"{Database.SqlContext.SqlSyntax.GetType()} is not of type {typeof(ISqlSyntaxProvider2)}"); } @@ -765,7 +746,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)}"); } 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); 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" diff --git a/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs b/src/Umbraco.Tests/Scoping/ScopeUnitTests.cs index 32bd7e2afe..038376f71c 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; @@ -9,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 { @@ -72,6 +74,30 @@ 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(); + } + + 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,31 +202,58 @@ 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(); + } + + 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] 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.AreEqual(2, realOuterScope.WriteLocks[realOuterScope.InstanceId][Constants.Locks.ContentTree]); + Assert.IsFalse(realOuterScope.WriteLocks.ContainsKey(innerscopeId)); outerscope.Complete(); } } @@ -209,27 +262,32 @@ 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.AreEqual(2, realOuterScope.ReadLocks[outerscope.InstanceId][Constants.Locks.ContentTree]); + Assert.IsFalse(realOuterScope.ReadLocks.ContainsKey(innerscopeId)); + + outerscope.Complete(); } } @@ -238,51 +296,61 @@ 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], $"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.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(innerScope2Id)); + Assert.IsFalse(realParentScope.WriteLocks.ContainsKey(innerScope1Id)); - outerScope.Complete(); + parentScope.Complete(); } } @@ -290,48 +358,166 @@ 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(innerScope2Id)); + Assert.IsFalse(realParentScope.ReadLocks.ContainsKey(innerScope1Id)); + + 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.IsFalse(realScope.WriteLocks[scope.InstanceId].ContainsKey(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.IsFalse(realScope.ReadLocks[scope.InstanceId].ContainsKey(Constants.Locks.Languages)); + scope.Complete(); + } + } + + [Test] + public void Scope_Throws_If_ReadLocks_Not_Cleared() + { + var scopeprovider = GetScopeProvider(out var syntaxProviderMock); + var scope = (Scope) scopeprovider.CreateScope(); + + try + { + // Request a lock to create the ReadLocks dict. + scope.ReadLock(Constants.Locks.Domains); + + var readDict = new Dictionary(); + readDict[Constants.Locks.Languages] = 1; + scope.ReadLocks[Guid.NewGuid()] = readDict; + + 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] + public void Scope_Throws_If_WriteLocks_Not_Cleared() + { + var scopeprovider = GetScopeProvider(out var syntaxProviderMock); + var scope = (Scope) scopeprovider.CreateScope(); + + try + { + // Request a lock to create the WriteLocks dict. + scope.WriteLock(Constants.Locks.Domains); + + var writeDict = new Dictionary(); + writeDict[Constants.Locks.Languages] = 1; + scope.WriteLocks[Guid.NewGuid()] = writeDict; + + 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); } } }