From 21126b7ba6b95ea10e4099633ee0e4f4fcd1cee9 Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Mon, 4 Mar 2024 12:50:24 +0100 Subject: [PATCH 01/22] Localize validation messages with user lang files (#15820) * Localize validation messages with user lang files * Fixed unit tests --- .../Mapping/ContentPropertyDisplayMapper.cs | 2 ++ .../Services/PropertyValidationService.cs | 20 +++++++++++++++++-- .../Services/ContentServiceTests.cs | 3 ++- .../Umbraco.Core/Models/VariationTests.cs | 4 +++- .../PropertyValidationServiceTests.cs | 3 ++- 5 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/Models/Mapping/ContentPropertyDisplayMapper.cs b/src/Umbraco.Core/Models/Mapping/ContentPropertyDisplayMapper.cs index 237f1f2f40..a3c16ba148 100644 --- a/src/Umbraco.Core/Models/Mapping/ContentPropertyDisplayMapper.cs +++ b/src/Umbraco.Core/Models/Mapping/ContentPropertyDisplayMapper.cs @@ -86,5 +86,7 @@ internal class ContentPropertyDisplayMapper : ContentPropertyBasicMapper()) + { + } + + public PropertyValidationService( + PropertyEditorCollection propertyEditors, + IDataTypeService dataTypeService, + ILocalizedTextService textService, + IValueEditorCache valueEditorCache, + ICultureDictionary cultureDictionary) { _propertyEditors = propertyEditors; _dataTypeService = dataTypeService; _textService = textService; _valueEditorCache = valueEditorCache; + _cultureDictionary = cultureDictionary; } /// @@ -76,13 +92,13 @@ public class PropertyValidationService : IPropertyValidationService if (isRequired && !string.IsNullOrWhiteSpace(isRequiredMessage) && requiredDefaultMessages.Contains(validationResult.ErrorMessage, StringComparer.OrdinalIgnoreCase)) { - validationResult.ErrorMessage = isRequiredMessage; + validationResult.ErrorMessage = _textService.UmbracoDictionaryTranslate(_cultureDictionary, isRequiredMessage); } if (!string.IsNullOrWhiteSpace(validationRegExp) && !string.IsNullOrWhiteSpace(validationRegExpMessage) && formatDefaultMessages.Contains(validationResult.ErrorMessage, StringComparer.OrdinalIgnoreCase)) { - validationResult.ErrorMessage = validationRegExpMessage; + validationResult.ErrorMessage = _textService.UmbracoDictionaryTranslate(_cultureDictionary, validationRegExpMessage); } yield return validationResult; diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceTests.cs index aba329404a..a624d6d885 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceTests.cs @@ -7,6 +7,7 @@ using Moq; using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Dictionary; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Notifications; @@ -1195,7 +1196,7 @@ public class ContentServiceTests : UmbracoIntegrationTestWithContent // content cannot publish values because they are invalid var propertyValidationService = new PropertyValidationService(PropertyEditorCollection, DataTypeService, - TextService, ValueEditorCache); + TextService, ValueEditorCache, Mock.Of()); var isValid = propertyValidationService.IsPropertyDataValid(content, out var invalidProperties, CultureImpact.Invariant); Assert.IsFalse(isValid); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/VariationTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/VariationTests.cs index ca2ae76428..06edf59d3e 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/VariationTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/VariationTests.cs @@ -5,6 +5,7 @@ using Moq; using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Dictionary; using Umbraco.Cms.Core.IO; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.PropertyEditors; @@ -615,6 +616,7 @@ public class VariationTests propertyEditorCollection, dataTypeService, localizedTextService, - new ValueEditorCache()); + new ValueEditorCache(), + Mock.Of()); } } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Services/PropertyValidationServiceTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Services/PropertyValidationServiceTests.cs index 736e124324..160e9e7caa 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Services/PropertyValidationServiceTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Services/PropertyValidationServiceTests.cs @@ -6,6 +6,7 @@ using Moq; using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Dictionary; using Umbraco.Cms.Core.IO; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.PropertyEditors; @@ -51,7 +52,7 @@ public class PropertyValidationServiceTests var propEditors = new PropertyEditorCollection(new DataEditorCollection(() => new[] { dataEditor })); - validationService = new PropertyValidationService(propEditors, dataTypeService.Object, Mock.Of(), new ValueEditorCache()); + validationService = new PropertyValidationService(propEditors, dataTypeService.Object, Mock.Of(), new ValueEditorCache(), Mock.Of()); } [Test] From ca9a3bbf1b06c44e14c9aa51e0ec2b20561e53f3 Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Mon, 4 Mar 2024 13:17:38 +0100 Subject: [PATCH 02/22] Use same cache level for MNTP no matter the output channel (#15828) --- .../ValueConverters/MultiNodeTreePickerValueConverter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Core/PropertyEditors/ValueConverters/MultiNodeTreePickerValueConverter.cs b/src/Umbraco.Core/PropertyEditors/ValueConverters/MultiNodeTreePickerValueConverter.cs index 7d512fbb2b..3ddb98b40e 100644 --- a/src/Umbraco.Core/PropertyEditors/ValueConverters/MultiNodeTreePickerValueConverter.cs +++ b/src/Umbraco.Core/PropertyEditors/ValueConverters/MultiNodeTreePickerValueConverter.cs @@ -181,7 +181,7 @@ public class MultiNodeTreePickerValueConverter : PropertyValueConverterBase, IDe return source; } - public PropertyCacheLevel GetDeliveryApiPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Elements; + public PropertyCacheLevel GetDeliveryApiPropertyCacheLevel(IPublishedPropertyType propertyType) => PropertyCacheLevel.Snapshot; public PropertyCacheLevel GetDeliveryApiPropertyCacheLevelForExpansion(IPublishedPropertyType propertyType) => PropertyCacheLevel.Snapshot; From 7dff3a37553ad1003e8422872e17851b73321586 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Wed, 6 Mar 2024 11:01:37 +0100 Subject: [PATCH 03/22] bump version --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index 44ba7dcf8f..3c26ae287d 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-rc2", + "version": "13.2.0", "assemblyVersion": { "precision": "build" }, From b86eeb6b520e7e5003e367f0cb875973b28e0a0c Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Thu, 7 Mar 2024 09:54:14 +0100 Subject: [PATCH 04/22] bumb version --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index 3c26ae287d..f70819e8b4 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.3.0-rc", "assemblyVersion": { "precision": "build" }, From 90820483d5c009cc7075c7f1a641dae783b47f86 Mon Sep 17 00:00:00 2001 From: Elitsa Marinovska <21998037+elit0451@users.noreply.github.com> Date: Fri, 8 Mar 2024 08:36:00 +0100 Subject: [PATCH 05/22] Try to parse string as int and return id.ToString(CultureInfo.InvariantCulture) (#15848) --- src/Umbraco.Examine.Lucene/DeliveryApiContentIndex.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Examine.Lucene/DeliveryApiContentIndex.cs b/src/Umbraco.Examine.Lucene/DeliveryApiContentIndex.cs index a3508a3fed..c572fc85f7 100644 --- a/src/Umbraco.Examine.Lucene/DeliveryApiContentIndex.cs +++ b/src/Umbraco.Examine.Lucene/DeliveryApiContentIndex.cs @@ -1,9 +1,9 @@ +using System.Globalization; using Examine; using Examine.Lucene; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; -using Umbraco.Cms.Core; using Umbraco.Cms.Core.DeliveryApi; using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Hosting; @@ -125,8 +125,14 @@ public class DeliveryApiContentIndex : UmbracoExamineIndex private (string? ContentId, string? Culture) ParseItemId(string id) { + if (int.TryParse(id, out _)) + { + return (id, null); + } + DeliveryApiIndexCompositeIdModel compositeIdModel = _deliveryApiCompositeIdHandler.Decompose(id); - return (compositeIdModel.Id.ToString() ?? id, compositeIdModel.Culture); + + return (compositeIdModel.Id?.ToString(CultureInfo.InvariantCulture), compositeIdModel.Culture); } protected override void OnTransformingIndexValues(IndexingItemEventArgs e) From ed517ecd861fca7d64193db8a1f6e104aeb1e505 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 14 Mar 2024 13:31:30 +0000 Subject: [PATCH 06/22] Update Imagesharp (#15885) --- src/Directory.Packages.props | 103 +++++++++++++++++------------------ 1 file changed, 50 insertions(+), 53 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index a906a587ef..4d6bf155d3 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -4,67 +4,64 @@ true NU1507 - - - - - - - - - + + + + + + + + - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + - - - - - + + + + + - - - - - - - - - - - + + + + + + + + + + + - - - - - - - - + + + + + + + - - - + + + - - + \ No newline at end of file From 659e74d831e6646321f48dda21f97f026b173e53 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 08:08:27 +0100 Subject: [PATCH 07/22] Update imagesharp 3 --- src/Directory.Packages.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 152f7cb91e..15801621da 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -56,7 +56,7 @@ - + From 0e43c43609007786fd82e8993ed109f19e18bdee Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 08:09:39 +0100 Subject: [PATCH 08/22] Update imagesharp 3 --- src/Directory.Packages.props | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 15801621da..b149d1d611 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -56,8 +56,8 @@ - - + + From 7585918821f6e0e2f1c40a478736ac1b8a8b1a7e Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 08:20:50 +0100 Subject: [PATCH 09/22] Revert ImageSharp 3 upgrade due to breaking changes --- src/Directory.Packages.props | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index b149d1d611..15801621da 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -56,8 +56,8 @@ - - + + From d82e262e139538f5bff26199791c24c9e6af0ed2 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 08:22:53 +0100 Subject: [PATCH 10/22] Update imagesharp 3 --- Directory.Packages.props | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index c80183b9c1..02dbaba948 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -73,7 +73,7 @@ - + @@ -90,4 +90,4 @@ - \ No newline at end of file + From e03ff2661b9bbd5c36e79dda092616f85039ad72 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 09:35:19 +0100 Subject: [PATCH 11/22] Ignore test of lazy locks --- .../Umbraco.Infrastructure/Persistence/LocksTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs index 88b48dfaae..8ae6818aeb 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs @@ -125,6 +125,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() { From 3718a9cdd48ea4e57024b4c433cf2c55a2e91c53 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 09:47:18 +0100 Subject: [PATCH 12/22] Revert "Ignore test of lazy locks" This reverts commit e03ff2661b9bbd5c36e79dda092616f85039ad72. --- .../Umbraco.Infrastructure/Persistence/LocksTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs index 8ae6818aeb..88b48dfaae 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs @@ -125,7 +125,6 @@ public class LocksTests : UmbracoIntegrationTest } } - [NUnit.Framework.Ignore("We currently do not have a way to force lazy locks")] [Test] public void GivenNonEagerLocking_WhenNoDbIsAccessed_ThenNoSqlIsExecuted() { From 2c23e67c65a98e0573924cd1a55bf03df6e13410 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 09:56:02 +0000 Subject: [PATCH 13/22] 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() From 3e08ce1efb2a7f73ec84e8140585c0eff1d7cb30 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 15:58:22 +0100 Subject: [PATCH 14/22] Fix after 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 1fa779d221..171580407d 100644 --- a/src/Umbraco.Core/Scoping/LockingMechanism.cs +++ b/src/Umbraco.Core/Scoping/LockingMechanism.cs @@ -39,7 +39,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) => EagerReadLockInner(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 7e822bb8a1ffc2c344e3a92ee083b2bc3f64ee59 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Fri, 15 Mar 2024 15:58:22 +0100 Subject: [PATCH 15/22] Fix after merge (cherry picked from commit 3e08ce1efb2a7f73ec84e8140585c0eff1d7cb30) --- 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 1fa779d221..171580407d 100644 --- a/src/Umbraco.Core/Scoping/LockingMechanism.cs +++ b/src/Umbraco.Core/Scoping/LockingMechanism.cs @@ -39,7 +39,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) => EagerReadLockInner(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 b743f6a2df7c4e8bc72d6aaffd2ae1544ed2ad1a Mon Sep 17 00:00:00 2001 From: Jey Date: Mon, 18 Mar 2024 08:27:41 +0100 Subject: [PATCH 16/22] Merge pull request from GHSA-552f-97wf-pmpq Co-authored-by: jey --- src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs b/src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs index 96f0025efa..231f2b3b1a 100644 --- a/src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs +++ b/src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs @@ -134,8 +134,8 @@ public abstract class UmbracoUserManager : UserManager public override async Task CheckPasswordAsync(TUser user, string? password) { - // we cannot proceed if the user passed in does not have an identity - if (user.HasIdentity == false) + // we cannot proceed if the user passed in does not have an identity, or if no password is provided. + if (user.HasIdentity == false || password is null) { return false; } @@ -252,7 +252,7 @@ public abstract class UmbracoUserManager : UserManager ValidateCredentialsAsync(string username, string password) { TUser user = await FindByNameAsync(username); - + if (user == null) { return false; @@ -263,7 +263,7 @@ public abstract class UmbracoUserManager : UserManager)); } - + var result = await VerifyPasswordAsync(userPasswordStore, user, password); return result == PasswordVerificationResult.Success || result == PasswordVerificationResult.SuccessRehashNeeded; From 352b6fa45b4f8a745ca95a9e02a8c51f84a9dc5f Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Mon, 18 Mar 2024 08:39:19 +0100 Subject: [PATCH 17/22] 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 18/22] 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 19/22] 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 e267b4157582efbbc1b8ab2a7d41fc065c36368f Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Mon, 18 Mar 2024 12:39:24 +0100 Subject: [PATCH 20/22] 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 --- .../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 a39030f4ba8825545e424e0660b1ff3ce3c93f51 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Mon, 18 Mar 2024 12:39:24 +0100 Subject: [PATCH 21/22] 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 22/22] 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);