From 2c23e67c65a98e0573924cd1a55bf03df6e13410 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 09:56:02 +0000 Subject: [PATCH] Fixing locking issues for document type saves. (#15854) * Added ExecuteNonQuery(DbCommand command) on database to ensure we call OnExecutingCommand and OnExecutedCommand when executing DbCommands * Added Cache Instructions lock, to avoid deadlocks * Optimized read locks for nucache when only one content type is rebuilt * Optimized the SqlServer locks, so only one command is executed (and thereby roundtrip) per lock instead of two * Avoid breaking changes * Cosmetic changes * Take locks if everything is rebuild * Use same lock in scopes, to avoid potential deadlocks between the two * Use eager locks in PublishedSnapshotService.cs * Added timeouts to some of the application locks * Revert "Use eager locks in PublishedSnapshotService.cs" This reverts commit 01873aae978ffa6e6686d253e482c493715e3a96. * Revert "Added Cache Instructions lock, to avoid deadlocks" This reverts commit e3fca7c12a804bb32ca1156b8abd42a957e9dc21. * Use single readlock call to lock many * Use eager locks for reads * Eager write locks * Ignore test of lazy locks * Unique timeout exception messages --------- Co-authored-by: kjac --- .../SqlServerDistributedLockingMechanism.cs | 10 +++-- .../SqliteDistributedLockingMechanism.cs | 2 +- src/Umbraco.Core/Cache/ObjectCacheAppCache.cs | 39 +++++++++++++++---- .../Persistence/IUmbracoDatabase.cs | 4 ++ .../Persistence/UmbracoDatabase.cs | 8 ++++ src/Umbraco.Infrastructure/Scoping/Scope.cs | 26 ++++++------- .../ContentStore.cs | 9 ++++- .../Persistence/NuCacheContentService.cs | 22 +++++++++-- .../SnapDictionary.cs | 9 ++++- .../Persistence/LocksTests.cs | 38 +++++++++++++++--- 10 files changed, 129 insertions(+), 38 deletions(-) diff --git a/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerDistributedLockingMechanism.cs b/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerDistributedLockingMechanism.cs index 358eae2e38..e0bf4767bf 100644 --- a/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerDistributedLockingMechanism.cs +++ b/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerDistributedLockingMechanism.cs @@ -134,9 +134,10 @@ public class SqlServerDistributedLockingMechanism : IDistributedLockingMechanism const string query = "SELECT value FROM umbracoLock WITH (REPEATABLEREAD) WHERE id=@id"; - db.Execute("SET LOCK_TIMEOUT " + _timeout.TotalMilliseconds + ";"); + var lockTimeoutQuery = $"SET LOCK_TIMEOUT {_timeout.TotalMilliseconds}"; - var i = db.ExecuteScalar(query, new { id = LockId }); + // execute the lock timeout query and the actual query in a single server roundtrip + var i = db.ExecuteScalar($"{lockTimeoutQuery};{query}", new { id = LockId }); if (i == null) { @@ -169,9 +170,10 @@ public class SqlServerDistributedLockingMechanism : IDistributedLockingMechanism const string query = @"UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id=@id"; - db.Execute("SET LOCK_TIMEOUT " + _timeout.TotalMilliseconds + ";"); + var lockTimeoutQuery = $"SET LOCK_TIMEOUT {_timeout.TotalMilliseconds}"; - var i = db.Execute(query, new { id = LockId }); + // execute the lock timeout query and the actual query in a single server roundtrip + var i = db.Execute($"{lockTimeoutQuery};{query}", new { id = LockId }); if (i == 0) { diff --git a/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteDistributedLockingMechanism.cs b/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteDistributedLockingMechanism.cs index c3d6f8b29d..9f25f77c2c 100644 --- a/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteDistributedLockingMechanism.cs +++ b/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteDistributedLockingMechanism.cs @@ -154,7 +154,7 @@ public class SqliteDistributedLockingMechanism : IDistributedLockingMechanism try { - var i = command.ExecuteNonQuery(); + var i = db.ExecuteNonQuery(command); if (i == 0) { diff --git a/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs b/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs index dcd83ece94..ff1c9b3bfd 100644 --- a/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs +++ b/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs @@ -9,6 +9,9 @@ namespace Umbraco.Cms.Core.Cache; /// public class ObjectCacheAppCache : IAppPolicyCache, IDisposable { + private static readonly TimeSpan _readLockTimeout = TimeSpan.FromSeconds(5); + private static readonly TimeSpan _writeLockTimeout = TimeSpan.FromSeconds(5); + private readonly ReaderWriterLockSlim _locker = new(LockRecursionPolicy.SupportsRecursion); private bool _disposedValue; @@ -33,7 +36,10 @@ public class ObjectCacheAppCache : IAppPolicyCache, IDisposable Lazy? result; try { - _locker.EnterReadLock(); + if (_locker.TryEnterReadLock(_readLockTimeout) is false) + { + throw new TimeoutException("Timeout exceeded to the memory cache when getting item"); + } result = MemoryCache.Get(key) as Lazy; // null if key not found } finally @@ -195,7 +201,10 @@ public class ObjectCacheAppCache : IAppPolicyCache, IDisposable { try { - _locker.EnterWriteLock(); + if (_locker.TryEnterWriteLock(_writeLockTimeout) is false) + { + throw new TimeoutException("Timeout exceeded to the memory cache when clearing item"); + } if (MemoryCache[key] == null) { return; @@ -223,8 +232,10 @@ public class ObjectCacheAppCache : IAppPolicyCache, IDisposable var isInterface = type.IsInterface; try { - _locker.EnterWriteLock(); - + if (_locker.TryEnterWriteLock(_writeLockTimeout) is false) + { + throw new TimeoutException("Timeout exceeded to the memory cache when clearing by type"); + } // ToArray required to remove foreach (var key in MemoryCache .Where(x => @@ -259,7 +270,10 @@ public class ObjectCacheAppCache : IAppPolicyCache, IDisposable { try { - _locker.EnterWriteLock(); + if (_locker.TryEnterWriteLock(_writeLockTimeout) is false) + { + throw new TimeoutException("Timeout exceeded to the memory cache when clearing by generic type"); + } Type typeOfT = typeof(T); var isInterface = typeOfT.IsInterface; @@ -296,7 +310,10 @@ public class ObjectCacheAppCache : IAppPolicyCache, IDisposable { try { - _locker.EnterWriteLock(); + if (_locker.TryEnterWriteLock(_writeLockTimeout) is false) + { + throw new TimeoutException("Timeout exceeded to the memory cache when clearing generic type with predicate"); + } Type typeOfT = typeof(T); var isInterface = typeOfT.IsInterface; @@ -338,7 +355,10 @@ public class ObjectCacheAppCache : IAppPolicyCache, IDisposable { try { - _locker.EnterWriteLock(); + if (_locker.TryEnterWriteLock(_writeLockTimeout) is false) + { + throw new TimeoutException("Timeout exceeded to the memory cache when clearing with prefix"); + } // ToArray required to remove foreach (var key in MemoryCache @@ -365,7 +385,10 @@ public class ObjectCacheAppCache : IAppPolicyCache, IDisposable try { - _locker.EnterWriteLock(); + if (_locker.TryEnterWriteLock(_writeLockTimeout) is false) + { + throw new TimeoutException("Timeout exceeded to the memory cach when clearing by regex"); + } // ToArray required to remove foreach (var key in MemoryCache diff --git a/src/Umbraco.Infrastructure/Persistence/IUmbracoDatabase.cs b/src/Umbraco.Infrastructure/Persistence/IUmbracoDatabase.cs index 431ddeb5e8..b86648f05c 100644 --- a/src/Umbraco.Infrastructure/Persistence/IUmbracoDatabase.cs +++ b/src/Umbraco.Infrastructure/Persistence/IUmbracoDatabase.cs @@ -1,3 +1,4 @@ +using System.Data.Common; using NPoco; using Umbraco.Cms.Infrastructure.Migrations.Install; @@ -33,4 +34,7 @@ public interface IUmbracoDatabase : IDatabase bool IsUmbracoInstalled(); DatabaseSchemaResult ValidateSchema(); + + /// The number of rows affected. + int ExecuteNonQuery(DbCommand command) => command.ExecuteNonQuery(); } diff --git a/src/Umbraco.Infrastructure/Persistence/UmbracoDatabase.cs b/src/Umbraco.Infrastructure/Persistence/UmbracoDatabase.cs index 425629dc4b..74f8c297ef 100644 --- a/src/Umbraco.Infrastructure/Persistence/UmbracoDatabase.cs +++ b/src/Umbraco.Infrastructure/Persistence/UmbracoDatabase.cs @@ -224,6 +224,14 @@ public class UmbracoDatabase : Database, IUmbracoDatabase return databaseSchemaValidationResult ?? new DatabaseSchemaResult(); } + public int ExecuteNonQuery(DbCommand command) + { + OnExecutingCommand(command); + var i = command.ExecuteNonQuery(); + OnExecutedCommand(command); + return i; + } + /// /// Returns true if Umbraco database tables are detected to be installed /// diff --git a/src/Umbraco.Infrastructure/Scoping/Scope.cs b/src/Umbraco.Infrastructure/Scoping/Scope.cs index 000b6a602e..0db9656fc5 100644 --- a/src/Umbraco.Infrastructure/Scoping/Scope.cs +++ b/src/Umbraco.Infrastructure/Scoping/Scope.cs @@ -23,10 +23,9 @@ namespace Umbraco.Cms.Infrastructure.Scoping private readonly bool _autoComplete; private readonly CoreDebugSettings _coreDebugSettings; - private readonly object _dictionaryLocker; private readonly IEventAggregator _eventAggregator; private readonly IsolationLevel _isolationLevel; - private readonly object _lockQueueLocker = new(); + private readonly object _locker = new(); private readonly ILogger _logger; private readonly MediaFileManager _mediaFileManager; private readonly RepositoryCacheMode _repositoryCacheMode; @@ -87,7 +86,6 @@ namespace Umbraco.Cms.Infrastructure.Scoping _scopeFileSystem = scopeFileSystems; _autoComplete = autoComplete; Detachable = detachable; - _dictionaryLocker = new object(); #if DEBUG_SCOPES _scopeProvider.RegisterScope(this); @@ -562,7 +560,7 @@ namespace Umbraco.Cms.Infrastructure.Scoping DisposeLastScope(); } - lock (_lockQueueLocker) + lock (_locker) { _queuedLocks?.Clear(); } @@ -573,24 +571,24 @@ namespace Umbraco.Cms.Infrastructure.Scoping public void EagerReadLock(params int[] lockIds) => EagerReadLockInner(InstanceId, null, lockIds); /// - public void ReadLock(params int[] lockIds) => LazyReadLockInner(InstanceId, lockIds); + public void ReadLock(params int[] lockIds) => EagerReadLockInner(InstanceId, null, lockIds); public void EagerReadLock(TimeSpan timeout, int lockId) => EagerReadLockInner(InstanceId, timeout, lockId); /// - public void ReadLock(TimeSpan timeout, int lockId) => LazyReadLockInner(InstanceId, timeout, lockId); + public void ReadLock(TimeSpan timeout, int lockId) => EagerReadLockInner(InstanceId, timeout, lockId); public void EagerWriteLock(params int[] lockIds) => EagerWriteLockInner(InstanceId, null, lockIds); /// - public void WriteLock(params int[] lockIds) => LazyWriteLockInner(InstanceId, lockIds); + public void WriteLock(params int[] lockIds) => EagerWriteLockInner(InstanceId, null, lockIds); public void EagerWriteLock(TimeSpan timeout, int lockId) => EagerWriteLockInner(InstanceId, timeout, lockId); /// - public void WriteLock(TimeSpan timeout, int lockId) => LazyWriteLockInner(InstanceId, timeout, lockId); + public void WriteLock(TimeSpan timeout, int lockId) => EagerWriteLockInner(InstanceId, timeout, lockId); /// /// Used for testing. Ensures and gets any queued read locks. @@ -659,7 +657,7 @@ namespace Umbraco.Cms.Infrastructure.Scoping } else { - lock (_lockQueueLocker) + lock (_locker) { if (_queuedLocks?.Count > 0) { @@ -970,7 +968,7 @@ namespace Umbraco.Cms.Infrastructure.Scoping } else { - lock (_dictionaryLocker) + lock (_locker) { _readLocksDictionary?.Remove(instanceId); _writeLocksDictionary?.Remove(instanceId); @@ -1045,7 +1043,7 @@ namespace Umbraco.Cms.Infrastructure.Scoping private void LazyLockInner(DistributedLockType lockType, Guid instanceId, params int[] lockIds) { - lock (_lockQueueLocker) + lock (_locker) { if (_queuedLocks == null) { @@ -1061,7 +1059,7 @@ namespace Umbraco.Cms.Infrastructure.Scoping private void LazyLockInner(DistributedLockType lockType, Guid instanceId, TimeSpan timeout, int lockId) { - lock (_lockQueueLocker) + lock (_locker) { if (_queuedLocks == null) { @@ -1088,7 +1086,7 @@ namespace Umbraco.Cms.Infrastructure.Scoping } else { - lock (_dictionaryLocker) + lock (_locker) { foreach (var lockId in lockIds) { @@ -1122,7 +1120,7 @@ namespace Umbraco.Cms.Infrastructure.Scoping } else { - lock (_dictionaryLocker) + lock (_locker) { foreach (var lockId in lockIds) { diff --git a/src/Umbraco.PublishedCache.NuCache/ContentStore.cs b/src/Umbraco.PublishedCache.NuCache/ContentStore.cs index f38b9dd2fc..0230032dc2 100644 --- a/src/Umbraco.PublishedCache.NuCache/ContentStore.cs +++ b/src/Umbraco.PublishedCache.NuCache/ContentStore.cs @@ -27,6 +27,8 @@ namespace Umbraco.Cms.Infrastructure.PublishedCache; /// public class ContentStore { + private static readonly TimeSpan _monitorTimeout = TimeSpan.FromSeconds(30); + // TODO: collection trigger (ok for now) // see SnapDictionary notes private const long CollectMinGenDelta = 8; @@ -330,7 +332,12 @@ public class ContentStore throw new InvalidOperationException("Recursive locks not allowed"); } - Monitor.Enter(_wlocko, ref lockInfo.Taken); + Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken); + + if (Monitor.IsEntered(_wlocko) is false) + { + throw new TimeoutException("Could not enter monitor before timeout in content store"); + } lock (_rlocko) { diff --git a/src/Umbraco.PublishedCache.NuCache/Persistence/NuCacheContentService.cs b/src/Umbraco.PublishedCache.NuCache/Persistence/NuCacheContentService.cs index 09855f5682..13e911f137 100644 --- a/src/Umbraco.PublishedCache.NuCache/Persistence/NuCacheContentService.cs +++ b/src/Umbraco.PublishedCache.NuCache/Persistence/NuCacheContentService.cs @@ -127,9 +127,25 @@ public class NuCacheContentService : RepositoryService, INuCacheContentService { using (ICoreScope scope = ScopeProvider.CreateCoreScope(repositoryCacheMode: RepositoryCacheMode.Scoped)) { - scope.ReadLock(Constants.Locks.ContentTree); - scope.ReadLock(Constants.Locks.MediaTree); - scope.ReadLock(Constants.Locks.MemberTree); + if (contentTypeIds is null && mediaTypeIds is null && memberTypeIds is null) + { + scope.ReadLock(Constants.Locks.ContentTree,Constants.Locks.MediaTree,Constants.Locks.MemberTree); + } + + if (contentTypeIds is not null && contentTypeIds.Any()) + { + scope.ReadLock(Constants.Locks.ContentTree); + } + + if (mediaTypeIds is not null && mediaTypeIds.Any()) + { + scope.ReadLock(Constants.Locks.MediaTree); + } + + if (memberTypeIds is not null && memberTypeIds.Any()) + { + scope.ReadLock(Constants.Locks.MemberTree); + } _repository.Rebuild(contentTypeIds, mediaTypeIds, memberTypeIds); diff --git a/src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs b/src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs index 0d042380d2..b6c87e22bb 100644 --- a/src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs +++ b/src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs @@ -9,6 +9,8 @@ public class SnapDictionary where TValue : class where TKey : notnull { + private static readonly TimeSpan _monitorTimeout = TimeSpan.FromSeconds(30); + // minGenDelta to be adjusted // we may want to throttle collects even if delta is reached // we may want to force collect if delta is not reached but very old @@ -198,7 +200,12 @@ public class SnapDictionary throw new InvalidOperationException("Recursive locks not allowed"); } - Monitor.Enter(_wlocko, ref lockInfo.Taken); + Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken); + + if (Monitor.IsEntered(_wlocko) is false) + { + throw new TimeoutException("Could not enter the monitor before timeout in SnapDictionary"); + } lock (_rlocko) { diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs index a7390ba6ef..0d1b464efa 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs @@ -1,9 +1,4 @@ -using System; -using System.Collections.Generic; -using System.Linq; using System.Text; -using System.Threading; -using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using NPoco; @@ -15,7 +10,6 @@ using Umbraco.Cms.Persistence.Sqlite.Interceptors; using Umbraco.Cms.Tests.Common.Attributes; using Umbraco.Cms.Tests.Common.Testing; using Umbraco.Cms.Tests.Integration.Testing; -using Umbraco.Extensions; namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence; @@ -126,6 +120,7 @@ public class LocksTests : UmbracoIntegrationTest } } + [NUnit.Framework.Ignore("We currently do not have a way to force lazy locks")] [Test] public void GivenNonEagerLocking_WhenNoDbIsAccessed_ThenNoSqlIsExecuted() { @@ -155,6 +150,37 @@ public class LocksTests : UmbracoIntegrationTest Assert.AreEqual(0, sqlCount); } + [Test] + public void GivenNonEagerLocking_WhenDbIsAccessed_ThenSqlIsExecuted() + { + var sqlCount = 0; + + using (var scope = ScopeProvider.CreateScope()) + { + var db = ScopeAccessor.AmbientScope.Database; + try + { + db.EnableSqlCount = true; + + // Issue a lock request, but we are using non-eager + // locks so this only queues the request. + // The lock will not be issued unless we resolve + // scope.Database + scope.WriteLock(Constants.Locks.Servers); + + scope.Database.ExecuteScalar("SELECT 1"); + + sqlCount = db.SqlCount; + } + finally + { + db.EnableSqlCount = false; + } + } + + Assert.AreEqual(2,sqlCount); + } + [Test] [LongRunning] public void ConcurrentWritersTest()