Skip to content

Fix ContentStore locking exceptions in async code #17246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bergmania merged 4 commits into from
Oct 11, 2024
Merged

Fix ContentStore locking exceptions in async code #17246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ee6529
Add ContentCache test
ronaldbarendse Oct 10, 2024
970326b
Use SemaphoreSlim as write lock
ronaldbarendse Oct 10, 2024
0864c14
Apply lock imrpovements to SnapDictionary
ronaldbarendse Oct 11, 2024
cd73c8a
Obsolete unused MonitorLock
ronaldbarendse Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Umbraco.Core/MonitorLock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
25 changes: 14 additions & 11 deletions src/Umbraco.PublishedCache.NuCache/ContentStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -319,22 +319,24 @@ public override void Release(bool completed)

private void EnsureLocked()
{
if (!Monitor.IsEntered(_wlocko))
if (_writeLock.CurrentCount != 0)
{
throw new InvalidOperationException("Write lock must be acquried.");
}
}

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");
}
Expand All @@ -344,6 +346,7 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
// see SnapDictionary
try
{
// Run all code in finally to ensure ThreadAbortException does not interrupt execution
}
finally
{
Expand Down Expand Up @@ -374,6 +377,7 @@ private void Release(WriteLockInfo lockInfo, bool commit = true)
// see SnapDictionary
try
{
// Run all code in finally to ensure ThreadAbortException does not interrupt execution
}
finally
{
Expand Down Expand Up @@ -409,7 +413,7 @@ private void Release(WriteLockInfo lockInfo, bool commit = true)
{
if (lockInfo.Taken)
{
Monitor.Exit(_wlocko);
_writeLock.Release();
}
}
}
Expand Down Expand Up @@ -1817,7 +1821,7 @@ public Snapshot CreateSnapshot()
// 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;
Expand All @@ -1829,8 +1833,7 @@ public Snapshot CreateSnapshot()
}
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
Expand Down
82 changes: 43 additions & 39 deletions src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -187,22 +183,24 @@ public ScopedWriteLock(SnapDictionary<TKey, TValue> dictionary, bool scoped)

private void EnsureLocked()
{
if (!Monitor.IsEntered(_wlocko))
if (_writeLock.CurrentCount != 0)
{
throw new InvalidOperationException("Write lock must be acquried.");
}
}

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");
}
Expand All @@ -217,6 +215,7 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
// RuntimeHelpers.PrepareConstrainedRegions();
try
{
// Run all code in finally to ensure ThreadAbortException does not interrupt execution
}
finally
{
Expand Down Expand Up @@ -244,43 +243,48 @@ private void Release(WriteLockInfo lockInfo, bool commit = true)
return;
}

if (commit == false)
try
{
lock (_rlocko)
if (commit == false)
{
try
{
}
finally
lock (_rlocko)
{
// forget about the temp. liveGen
_nextGen = false;
_liveGen -= 1;
try
{
// Run all code in finally to ensure ThreadAbortException does not interrupt execution
}
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)
foreach (KeyValuePair<TKey, LinkedNode<TValue>> item in _items)
{
continue;
}
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);
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
Expand Down Expand Up @@ -434,7 +438,7 @@ public Snapshot CreateSnapshot()
// 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;
Expand Down Expand Up @@ -624,7 +628,7 @@ internal class TestHelper

public bool NextGen => _dict._nextGen;

public bool IsLocked => Monitor.IsEntered(_dict._wlocko);
public bool IsLocked => _dict._writeLock.CurrentCount == 0;

public bool CollectAuto
{
Expand Down
77 changes: 77 additions & 0 deletions tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PublishedCache/ContentCacheTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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));
}
}
}