From 352b6fa45b4f8a745ca95a9e02a8c51f84a9dc5f Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Mon, 18 Mar 2024 08:39:19 +0100 Subject: [PATCH 1/5] Bump version --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index 3c26ae287d..a2893b768e 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/main/src/NerdBank.GitVersioning/version.schema.json", - "version": "13.2.0", + "version": "13.2.1", "assemblyVersion": { "precision": "build" }, From 14f3336941801f85323a298050bc56bef9d917e0 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 10:56:02 +0100 Subject: [PATCH 2/5] 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 (cherry picked from commit 2c23e67c65a98e0573924cd1a55bf03df6e13410) --- .../SqlServerDistributedLockingMechanism.cs | 10 +++-- .../SqliteDistributedLockingMechanism.cs | 2 +- src/Umbraco.Core/Cache/ObjectCacheAppCache.cs | 39 +++++++++++++++---- .../Persistence/IUmbracoDatabase.cs | 4 ++ .../Persistence/UmbracoDatabase.cs | 8 ++++ .../ContentStore.cs | 9 ++++- .../Persistence/NuCacheContentService.cs | 22 +++++++++-- .../SnapDictionary.cs | 9 ++++- .../Persistence/LocksTests.cs | 35 +++++++++++++++-- 9 files changed, 117 insertions(+), 21 deletions(-) diff --git a/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerDistributedLockingMechanism.cs b/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerDistributedLockingMechanism.cs index cc2e1b5feb..77975e8f31 100644 --- a/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerDistributedLockingMechanism.cs +++ b/src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerDistributedLockingMechanism.cs @@ -143,9 +143,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) { @@ -178,9 +179,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 a29a4d1419..54e30d6fa6 100644 --- a/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteDistributedLockingMechanism.cs +++ b/src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteDistributedLockingMechanism.cs @@ -162,7 +162,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 af8eb8e1fe..3abec76ec2 100644 --- a/src/Umbraco.Infrastructure/Persistence/UmbracoDatabase.cs +++ b/src/Umbraco.Infrastructure/Persistence/UmbracoDatabase.cs @@ -223,6 +223,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.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 88b48dfaae..9e5b943e0c 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs @@ -1,8 +1,6 @@ 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; @@ -14,7 +12,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; @@ -125,6 +122,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() { @@ -154,6 +152,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() From 6e3496d3f53c55dbc4bb167431596a2afff9c6aa Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 15:58:22 +0100 Subject: [PATCH 3/5] Fix after merge (cherry picked from commit 3e08ce1efb2a7f73ec84e8140585c0eff1d7cb30) (cherry picked from commit 7e822bb8a1ffc2c344e3a92ee083b2bc3f64ee59) --- src/Umbraco.Core/Scoping/LockingMechanism.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Scoping/LockingMechanism.cs b/src/Umbraco.Core/Scoping/LockingMechanism.cs index e41fe2d874..b40de50cfe 100644 --- a/src/Umbraco.Core/Scoping/LockingMechanism.cs +++ b/src/Umbraco.Core/Scoping/LockingMechanism.cs @@ -40,7 +40,7 @@ public class LockingMechanism : ILockingMechanism public void ReadLock(Guid instanceId, params int[] lockIds) => ReadLock(instanceId, null, lockIds); /// - public void WriteLock(Guid instanceId, TimeSpan? timeout = null, params int[] lockIds) => LazyWriteLockInner(instanceId, timeout, lockIds); + public void WriteLock(Guid instanceId, TimeSpan? timeout = null, params int[] lockIds) => EagerWriteLockInner(instanceId, timeout, lockIds); public void WriteLock(Guid instanceId, params int[] lockIds) => WriteLock(instanceId, null, lockIds); From a39030f4ba8825545e424e0660b1ff3ce3c93f51 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Mon, 18 Mar 2024 12:39:24 +0100 Subject: [PATCH 4/5] DataTypeSplit datacollector should not care about propertyEditors (#15898) This ensures keys are correctly assumed to be unique => safe for dictionary usage Co-authored-by: Sven Geusens (cherry picked from commit e267b4157582efbbc1b8ab2a7d41fc065c36368f) --- .../Migrations/PreMigration/DataTypeSplitDataCollector.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Migrations/PreMigration/DataTypeSplitDataCollector.cs b/src/Umbraco.Infrastructure/Migrations/PreMigration/DataTypeSplitDataCollector.cs index 3a133bc253..3441c3b66c 100644 --- a/src/Umbraco.Infrastructure/Migrations/PreMigration/DataTypeSplitDataCollector.cs +++ b/src/Umbraco.Infrastructure/Migrations/PreMigration/DataTypeSplitDataCollector.cs @@ -92,7 +92,8 @@ public class DataTypeSplitDataCollector : INotificationHandler - de.GetType().Assembly.GetName().FullName + de.Type == EditorType.PropertyValue + && de.GetType().Assembly.GetName().FullName .StartsWith("umbraco.core", StringComparison.InvariantCultureIgnoreCase) is false && de.GetType().Assembly.GetName().FullName .StartsWith("umbraco.infrastructure", StringComparison.InvariantCultureIgnoreCase) is false) From 383617ec1711cb049d9a3154aa28cb11b5aacc9a Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Mon, 18 Mar 2024 19:00:30 +0100 Subject: [PATCH 5/5] Fix after wrong log merge --- src/Umbraco.Core/Scoping/LockingMechanism.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Scoping/LockingMechanism.cs b/src/Umbraco.Core/Scoping/LockingMechanism.cs index b40de50cfe..ad9fd6dd65 100644 --- a/src/Umbraco.Core/Scoping/LockingMechanism.cs +++ b/src/Umbraco.Core/Scoping/LockingMechanism.cs @@ -35,7 +35,7 @@ public class LockingMechanism : ILockingMechanism } /// - public void ReadLock(Guid instanceId, TimeSpan? timeout = null, params int[] lockIds) => LazyReadLockInner(instanceId, timeout, lockIds); + public void ReadLock(Guid instanceId, TimeSpan? timeout = null, params int[] lockIds) => EagerReadLockInner(instanceId, timeout, lockIds); public void ReadLock(Guid instanceId, params int[] lockIds) => ReadLock(instanceId, null, lockIds);