From 30b114d5389e1650aa462085a45aac0d7f42f4d5 Mon Sep 17 00:00:00 2001 From: Ronald Barendse Date: Fri, 11 Oct 2024 09:45:01 +0200 Subject: [PATCH] Fix ContentStore locking exceptions in async code (#17246) * Add ContentCache test * Use SemaphoreSlim as write lock * Apply lock imrpovements to SnapDictionary * Obsolete unused MonitorLock (cherry picked from commit c3db3457e7cb8e41c66673aee19246051ece3e80) --- src/Umbraco.Core/MonitorLock.cs | 1 + .../ContentStore.cs | 25 +++--- .../SnapDictionary.cs | 84 ++++++++++--------- .../PublishedCache/ContentCacheTests.cs | 77 +++++++++++++++++ 4 files changed, 136 insertions(+), 51 deletions(-) create mode 100644 tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PublishedCache/ContentCacheTests.cs diff --git a/src/Umbraco.Core/MonitorLock.cs b/src/Umbraco.Core/MonitorLock.cs index 45dbdbbd10..d8885ed256 100644 --- a/src/Umbraco.Core/MonitorLock.cs +++ b/src/Umbraco.Core/MonitorLock.cs @@ -4,6 +4,7 @@ namespace Umbraco.Cms.Core; /// Provides an equivalent to the c# lock statement, to be used in a using block. /// /// Ie replace lock (o) {...} by using (new MonitorLock(o)) { ... } +[Obsolete("Use System.Threading.Lock instead. This will be removed in a future version.")] public class MonitorLock : IDisposable { private readonly bool _entered; diff --git a/src/Umbraco.PublishedCache.NuCache/ContentStore.cs b/src/Umbraco.PublishedCache.NuCache/ContentStore.cs index 0230032dc2..d706301ff8 100644 --- a/src/Umbraco.PublishedCache.NuCache/ContentStore.cs +++ b/src/Umbraco.PublishedCache.NuCache/ContentStore.cs @@ -52,9 +52,9 @@ public class ContentStore // SnapDictionary has unit tests to ensure it all works correctly // For locking information, see SnapDictionary private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor; - private readonly object _rlocko = new(); private readonly IVariationContextAccessor _variationContextAccessor; - private readonly object _wlocko = new(); + private readonly object _rlocko = new(); + private readonly SemaphoreSlim _writeLock = new(1); private Task? _collectTask; private GenObj? _genObj; private long _liveGen; @@ -319,7 +319,7 @@ public class ContentStore private void EnsureLocked() { - if (!Monitor.IsEntered(_wlocko)) + if (_writeLock.CurrentCount != 0) { throw new InvalidOperationException("Write lock must be acquried."); } @@ -327,14 +327,16 @@ public class ContentStore private void Lock(WriteLockInfo lockInfo, bool forceGen = false) { - if (Monitor.IsEntered(_wlocko)) + if (_writeLock.CurrentCount == 0) { throw new InvalidOperationException("Recursive locks not allowed"); } - Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken); - - if (Monitor.IsEntered(_wlocko) is false) + if (_writeLock.Wait(_monitorTimeout)) + { + lockInfo.Taken = true; + } + else { throw new TimeoutException("Could not enter monitor before timeout in content store"); } @@ -344,6 +346,7 @@ public class ContentStore // see SnapDictionary try { + // Run all code in finally to ensure ThreadAbortException does not interrupt execution } finally { @@ -374,6 +377,7 @@ public class ContentStore // see SnapDictionary try { + // Run all code in finally to ensure ThreadAbortException does not interrupt execution } finally { @@ -409,7 +413,7 @@ public class ContentStore { if (lockInfo.Taken) { - Monitor.Exit(_wlocko); + _writeLock.Release(); } } } @@ -1817,7 +1821,7 @@ public class ContentStore // else we need to try to create a new gen ref // whether we are wlocked or not, noone can rlock while we do, // so _liveGen and _nextGen are safe - if (Monitor.IsEntered(_wlocko)) + if (_writeLock.CurrentCount == 0) { // write-locked, cannot use latest gen (at least 1) so use previous var snapGen = _nextGen ? _liveGen - 1 : _liveGen; @@ -1829,8 +1833,7 @@ public class ContentStore } else if (_genObj.Gen != snapGen) { - throw new PanicException( - $"The generation {_genObj.Gen} does not equal the snapshot generation {snapGen}"); + throw new PanicException($"The generation {_genObj.Gen} does not equal the snapshot generation {snapGen}"); } } else diff --git a/src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs b/src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs index b6c87e22bb..70bdcfe038 100644 --- a/src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs +++ b/src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs @@ -28,13 +28,9 @@ public class SnapDictionary // This class is optimized for many readers, few writers // Readers are lock-free - // NOTE - we used to lock _rlocko the long hand way with Monitor.Enter(_rlocko, ref lockTaken) but this has - // been replaced with a normal c# lock because that's exactly how the normal c# lock works, - // see https://blogs.msdn.microsoft.com/ericlippert/2009/03/06/locks-and-exceptions-do-not-mix/ - // for the readlock, there's no reason here to use the long hand way. private readonly ConcurrentDictionary> _items; private readonly object _rlocko = new(); - private readonly object _wlocko = new(); + private readonly SemaphoreSlim _writeLock = new(1); private Task? _collectTask; private GenObj? _genObj; private long _liveGen; @@ -187,7 +183,7 @@ public class SnapDictionary private void EnsureLocked() { - if (!Monitor.IsEntered(_wlocko)) + if (_writeLock.CurrentCount != 0) { throw new InvalidOperationException("Write lock must be acquried."); } @@ -195,14 +191,16 @@ public class SnapDictionary private void Lock(WriteLockInfo lockInfo, bool forceGen = false) { - if (Monitor.IsEntered(_wlocko)) + if (_writeLock.CurrentCount == 0) { throw new InvalidOperationException("Recursive locks not allowed"); } - Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken); - - if (Monitor.IsEntered(_wlocko) is false) + if (_writeLock.Wait(_monitorTimeout)) + { + lockInfo.Taken = true; + } + else { throw new TimeoutException("Could not enter the monitor before timeout in SnapDictionary"); } @@ -217,6 +215,7 @@ public class SnapDictionary // RuntimeHelpers.PrepareConstrainedRegions(); try { + // Run all code in finally to ensure ThreadAbortException does not interrupt execution } finally { @@ -244,43 +243,48 @@ public class SnapDictionary return; } - if (commit == false) + try { - lock (_rlocko) + if (commit == false) { - try + lock (_rlocko) { - } - finally - { - // forget about the temp. liveGen - _nextGen = false; - _liveGen -= 1; - } - } - - foreach (KeyValuePair> item in _items) - { - LinkedNode? link = item.Value; - if (link.Gen <= _liveGen) - { - continue; + try + { + // Run all code in finally to ensure ThreadAbortException does not interrupt execution + } + finally + { + // forget about the temp. liveGen + _nextGen = false; + _liveGen -= 1; + } } - TKey key = item.Key; - if (link.Next == null) + foreach (KeyValuePair> item in _items) { - _items.TryRemove(key, out link); - } - else - { - _items.TryUpdate(key, link.Next, link); + LinkedNode? link = item.Value; + if (link.Gen <= _liveGen) + { + continue; + } + + TKey key = item.Key; + if (link.Next == null) + { + _items.TryRemove(key, out link); + } + else + { + _items.TryUpdate(key, link.Next, link); + } } } } - - // TODO: Shouldn't this be in a finally block? - Monitor.Exit(_wlocko); + finally + { + _writeLock.Release(); + } } #endregion @@ -434,7 +438,7 @@ public class SnapDictionary // else we need to try to create a new gen object // whether we are wlocked or not, noone can rlock while we do, // so _liveGen and _nextGen are safe - if (Monitor.IsEntered(_wlocko)) + if (_writeLock.CurrentCount == 0) { // write-locked, cannot use latest gen (at least 1) so use previous var snapGen = _nextGen ? _liveGen - 1 : _liveGen; @@ -624,7 +628,7 @@ public class SnapDictionary public bool NextGen => _dict._nextGen; - public bool IsLocked => Monitor.IsEntered(_dict._wlocko); + public bool IsLocked => _dict._writeLock.CurrentCount == 0; public bool CollectAuto { diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PublishedCache/ContentCacheTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PublishedCache/ContentCacheTests.cs new file mode 100644 index 0000000000..6507bd6cda --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PublishedCache/ContentCacheTests.cs @@ -0,0 +1,77 @@ +using Microsoft.Extensions.Logging; +using NUnit.Framework; +using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.Hosting; +using Umbraco.Cms.Core.Models.PublishedContent; +using Umbraco.Cms.Core.PublishedCache; +using Umbraco.Cms.Infrastructure.PublishedCache; +using Umbraco.Cms.Infrastructure.PublishedCache.DataSource; +using Umbraco.Cms.Tests.Common.Builders; +using Umbraco.Cms.Tests.Common.Builders.Extensions; +using Umbraco.Cms.Tests.Common.Testing; +using Umbraco.Cms.Tests.Integration.Testing; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.PublishedCache; + +[TestFixture] +[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] +public class ContentCacheTests : UmbracoIntegrationTestWithContent +{ + private ContentStore GetContentStore() + { + var path = Path.Combine(GetRequiredService().LocalTempPath, "NuCache"); + Directory.CreateDirectory(path); + + var localContentDbPath = Path.Combine(path, "NuCache.Content.db"); + var localContentDbExists = File.Exists(localContentDbPath); + var contentDataSerializer = new ContentDataSerializer(new DictionaryOfPropertyDataSerializer()); + var localContentDb = BTree.GetTree(localContentDbPath, localContentDbExists, new NuCacheSettings(), contentDataSerializer); + + return new ContentStore( + GetRequiredService(), + GetRequiredService(), + LoggerFactory.CreateLogger(), + LoggerFactory, + GetRequiredService(), // new NoopPublishedModelFactory + localContentDb); + } + + private ContentNodeKit CreateContentNodeKit() + { + var contentData = new ContentDataBuilder() + .WithName("Content 1") + .WithProperties(new PropertyDataBuilder() + .WithPropertyData("welcomeText", "Welcome") + .WithPropertyData("welcomeText", "Welcome", "en-US") + .WithPropertyData("welcomeText", "Willkommen", "de") + .WithPropertyData("welcomeText", "Welkom", "nl") + .WithPropertyData("welcomeText2", "Welcome") + .WithPropertyData("welcomeText2", "Welcome", "en-US") + .WithPropertyData("noprop", "xxx") + .Build()) + .Build(); + + return ContentNodeKitBuilder.CreateWithContent( + ContentType.Id, + 1, + "-1,1", + draftData: contentData, + publishedData: contentData); + } + + [Test] + public async Task SetLocked() + { + var contentStore = GetContentStore(); + + using (contentStore.GetScopedWriteLock(ScopeProvider)) + { + var contentNodeKit = CreateContentNodeKit(); + + contentStore.SetLocked(contentNodeKit); + + // Try running the same operation again in an async task + await Task.Run(() => contentStore.SetLocked(contentNodeKit)); + } + } +}