yv01p/Umbraco-CMS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix ContentStore locking exceptions in async code (#17246)

Browse Source 
* Add ContentCache test

* Use SemaphoreSlim as write lock

* Apply lock imrpovements to SnapDictionary

* Obsolete unused MonitorLock

(cherry picked from commit c3db3457e7)
This commit is contained in:
Ronald Barendse
2024-10-11 09:45:01 +02:00
committed by Bjarke Berg
parent b814608c06
commit 30b114d538
4 changed files with 136 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
src/Umbraco.Core/MonitorLock.cs
View File

@@ -4,6 +4,7 @@ namespace Umbraco.Cms.Core;
/// Provides an equivalent to the c# lock statement, to be used in a using block.
/// </summary>
/// <remarks>Ie replace <c>lock (o) {...}</c> by <c>using (new MonitorLock(o)) { ... }</c></remarks>
[Obsolete("Use System.Threading.Lock instead. This will be removed in a future version.")]
public class MonitorLock : IDisposable
{
private readonly bool _entered;

25
src/Umbraco.PublishedCache.NuCache/ContentStore.cs
View File

@@ -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

84
src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs
View File

@@ -28,13 +28,9 @@ public class SnapDictionary<TKey, TValue>
// 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<TKey, LinkedNode<TValue>> _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<TKey, TValue>
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<TKey, TValue>
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<TKey, TValue>
// RuntimeHelpers.PrepareConstrainedRegions();
try
{
// Run all code in finally to ensure ThreadAbortException does not interrupt execution
}
finally
{
@@ -244,43 +243,48 @@ public class SnapDictionary<TKey, TValue>
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<TKey, LinkedNode<TValue>> item in _items)
{
LinkedNode<TValue>? 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<TKey, LinkedNode<TValue>> item in _items)
{
_items.TryRemove(key, out link);
}
else
{
_items.TryUpdate(key, link.Next, link);
LinkedNode<TValue>? 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<TKey, TValue>
// 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<TKey, TValue>
public bool NextGen => _dict._nextGen;
public bool IsLocked => Monitor.IsEntered(_dict._wlocko);
public bool IsLocked => _dict._writeLock.CurrentCount == 0;
public bool CollectAuto
{

77
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PublishedCache/ContentCacheTests.cs Normal file
View File

@@ -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<IHostingEnvironment>().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<IPublishedSnapshotAccessor>(),
GetRequiredService<IVariationContextAccessor>(),
LoggerFactory.CreateLogger<ContentCacheTests>(),
LoggerFactory,
GetRequiredService<IPublishedModelFactory>(), // 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));
}
}
}