Skip to content

Commit 378d4ec

Browse files
ronaldbarendsebergmania
ronaldbarendse
authored and
bergmania
committed
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 c3db345)
1 parent a497f3f commit 378d4ec

File tree

4 files changed

+135
-50
lines changed

4 files changed

+135
-50
lines changed

‎src/Umbraco.Core/MonitorLock.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ namespace Umbraco.Cms.Core;
44
/// Provides an equivalent to the c# lock statement, to be used in a using block.
55
/// </summary>
66
/// <remarks>Ie replace <c>lock (o) {...}</c> by <c>using (new MonitorLock(o)) { ... }</c></remarks>
7+
[Obsolete("Use System.Threading.Lock instead. This will be removed in a future version.")]
78
public class MonitorLock : IDisposable
89
{
910
private readonly bool _entered;

‎src/Umbraco.PublishedCache.NuCache/ContentStore.cs‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,9 @@ public class ContentStore
5252
// SnapDictionary has unit tests to ensure it all works correctly
5353
// For locking information, see SnapDictionary
5454
private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor;
55-
private readonly object _rlocko = new();
5655
private readonly IVariationContextAccessor _variationContextAccessor;
57-
private readonly object _wlocko = new();
56+
private readonly object _rlocko = new();
57+
private readonly SemaphoreSlim _writeLock = new(1);
5858
private Task? _collectTask;
5959
private GenObj? _genObj;
6060
private long _liveGen;
@@ -319,22 +319,24 @@ public override void Release(bool completed)
319319

320320
private void EnsureLocked()
321321
{
322-
if (!Monitor.IsEntered(_wlocko))
322+
if (_writeLock.CurrentCount != 0)
323323
{
324324
throw new InvalidOperationException("Write lock must be acquried.");
325325
}
326326
}
327327

328328
private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
329329
{
330-
if (Monitor.IsEntered(_wlocko))
330+
if (_writeLock.CurrentCount == 0)
331331
{
332332
throw new InvalidOperationException("Recursive locks not allowed");
333333
}
334334

335-
Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken);
336-
337-
if (Monitor.IsEntered(_wlocko) is false)
335+
if (_writeLock.Wait(_monitorTimeout))
336+
{
337+
lockInfo.Taken = true;
338+
}
339+
else
338340
{
339341
throw new TimeoutException("Could not enter monitor before timeout in content store");
340342
}
@@ -344,6 +346,7 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
344346
// see SnapDictionary
345347
try
346348
{
349+
// Run all code in finally to ensure ThreadAbortException does not interrupt execution
347350
}
348351
finally
349352
{
@@ -374,6 +377,7 @@ private void Release(WriteLockInfo lockInfo, bool commit = true)
374377
// see SnapDictionary
375378
try
376379
{
380+
// Run all code in finally to ensure ThreadAbortException does not interrupt execution
377381
}
378382
finally
379383
{
@@ -409,7 +413,7 @@ private void Release(WriteLockInfo lockInfo, bool commit = true)
409413
{
410414
if (lockInfo.Taken)
411415
{
412-
Monitor.Exit(_wlocko);
416+
_writeLock.Release();
413417
}
414418
}
415419
}
@@ -1817,7 +1821,7 @@ public Snapshot CreateSnapshot()
18171821
// else we need to try to create a new gen ref
18181822
// whether we are wlocked or not, noone can rlock while we do,
18191823
// so _liveGen and _nextGen are safe
1820-
if (Monitor.IsEntered(_wlocko))
1824+
if (_writeLock.CurrentCount == 0)
18211825
{
18221826
// write-locked, cannot use latest gen (at least 1) so use previous
18231827
var snapGen = _nextGen ? _liveGen - 1 : _liveGen;
@@ -1829,8 +1833,7 @@ public Snapshot CreateSnapshot()
18291833
}
18301834
else if (_genObj.Gen != snapGen)
18311835
{
1832-
throw new PanicException(
1833-
$"The generation {_genObj.Gen} does not equal the snapshot generation {snapGen}");
1836+
throw new PanicException($"The generation {_genObj.Gen} does not equal the snapshot generation {snapGen}");
18341837
}
18351838
}
18361839
else

‎src/Umbraco.PublishedCache.NuCache/SnapDictionary.cs‎

Lines changed: 43 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,9 @@ public class SnapDictionary<TKey, TValue>
2828
// This class is optimized for many readers, few writers
2929
// Readers are lock-free
3030

31-
// NOTE - we used to lock _rlocko the long hand way with Monitor.Enter(_rlocko, ref lockTaken) but this has
32-
// been replaced with a normal c# lock because that's exactly how the normal c# lock works,
33-
// see https://blogs.msdn.microsoft.com/ericlippert/2009/03/06/locks-and-exceptions-do-not-mix/
34-
// for the readlock, there's no reason here to use the long hand way.
3531
private readonly ConcurrentDictionary<TKey, LinkedNode<TValue>> _items;
3632
private readonly object _rlocko = new();
37-
private readonly object _wlocko = new();
33+
private readonly SemaphoreSlim _writeLock = new(1);
3834
private Task? _collectTask;
3935
private GenObj? _genObj;
4036
private long _liveGen;
@@ -187,22 +183,24 @@ public ScopedWriteLock(SnapDictionary<TKey, TValue> dictionary, bool scoped)
187183

188184
private void EnsureLocked()
189185
{
190-
if (!Monitor.IsEntered(_wlocko))
186+
if (_writeLock.CurrentCount != 0)
191187
{
192188
throw new InvalidOperationException("Write lock must be acquried.");
193189
}
194190
}
195191

196192
private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
197193
{
198-
if (Monitor.IsEntered(_wlocko))
194+
if (_writeLock.CurrentCount == 0)
199195
{
200196
throw new InvalidOperationException("Recursive locks not allowed");
201197
}
202198

203-
Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken);
204-
205-
if (Monitor.IsEntered(_wlocko) is false)
199+
if (_writeLock.Wait(_monitorTimeout))
200+
{
201+
lockInfo.Taken = true;
202+
}
203+
else
206204
{
207205
throw new TimeoutException("Could not enter the monitor before timeout in SnapDictionary");
208206
}
@@ -217,6 +215,7 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
217215
// RuntimeHelpers.PrepareConstrainedRegions();
218216
try
219217
{
218+
// Run all code in finally to ensure ThreadAbortException does not interrupt execution
220219
}
221220
finally
222221
{
@@ -244,43 +243,48 @@ private void Release(WriteLockInfo lockInfo, bool commit = true)
244243
return;
245244
}
246245

247-
if (commit == false)
246+
try
248247
{
249-
lock (_rlocko)
248+
if (commit == false)
250249
{
251-
try
252-
{
253-
}
254-
finally
250+
lock (_rlocko)
255251
{
256-
// forget about the temp. liveGen
257-
_nextGen = false;
258-
_liveGen -= 1;
252+
try
253+
{
254+
// Run all code in finally to ensure ThreadAbortException does not interrupt execution
255+
}
256+
finally
257+
{
258+
// forget about the temp. liveGen
259+
_nextGen = false;
260+
_liveGen -= 1;
261+
}
259262
}
260-
}
261263

262-
foreach (KeyValuePair<TKey, LinkedNode<TValue>> item in _items)
263-
{
264-
LinkedNode<TValue>? link = item.Value;
265-
if (link.Gen <= _liveGen)
264+
foreach (KeyValuePair<TKey, LinkedNode<TValue>> item in _items)
266265
{
267-
continue;
268-
}
266+
LinkedNode<TValue>? link = item.Value;
267+
if (link.Gen <= _liveGen)
268+
{
269+
continue;
270+
}
269271

270-
TKey key = item.Key;
271-
if (link.Next == null)
272-
{
273-
_items.TryRemove(key, out link);
274-
}
275-
else
276-
{
277-
_items.TryUpdate(key, link.Next, link);
272+
TKey key = item.Key;
273+
if (link.Next == null)
274+
{
275+
_items.TryRemove(key, out link);
276+
}
277+
else
278+
{
279+
_items.TryUpdate(key, link.Next, link);
280+
}
278281
}
279282
}
280283
}
281-
282-
// TODO: Shouldn't this be in a finally block?
283-
Monitor.Exit(_wlocko);
284+
finally
285+
{
286+
_writeLock.Release();
287+
}
284288
}
285289

286290
#endregion
@@ -434,7 +438,7 @@ public Snapshot CreateSnapshot()
434438
// else we need to try to create a new gen object
435439
// whether we are wlocked or not, noone can rlock while we do,
436440
// so _liveGen and _nextGen are safe
437-
if (Monitor.IsEntered(_wlocko))
441+
if (_writeLock.CurrentCount == 0)
438442
{
439443
// write-locked, cannot use latest gen (at least 1) so use previous
440444
var snapGen = _nextGen ? _liveGen - 1 : _liveGen;
@@ -624,7 +628,7 @@ internal class TestHelper
624628

625629
public bool NextGen => _dict._nextGen;
626630

627-
public bool IsLocked => Monitor.IsEntered(_dict._wlocko);
631+
public bool IsLocked => _dict._writeLock.CurrentCount == 0;
628632

629633
public bool CollectAuto
630634
{

‎tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PublishedCache/ContentCacheTests.cs‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
using Microsoft.Extensions.Logging;
2+
using NUnit.Framework;
3+
using Umbraco.Cms.Core.Configuration.Models;
4+
using Umbraco.Cms.Core.Hosting;
5+
using Umbraco.Cms.Core.Models.PublishedContent;
6+
using Umbraco.Cms.Core.PublishedCache;
7+
using Umbraco.Cms.Infrastructure.PublishedCache;
8+
using Umbraco.Cms.Infrastructure.PublishedCache.DataSource;
9+
using Umbraco.Cms.Tests.Common.Builders;
10+
using Umbraco.Cms.Tests.Common.Builders.Extensions;
11+
using Umbraco.Cms.Tests.Common.Testing;
12+
using Umbraco.Cms.Tests.Integration.Testing;
13+
14+
namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.PublishedCache;
15+
16+
[TestFixture]
17+
[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)]
18+
public class ContentCacheTests : UmbracoIntegrationTestWithContent
19+
{
20+
private ContentStore GetContentStore()
21+
{
22+
var path = Path.Combine(GetRequiredService<IHostingEnvironment>().LocalTempPath, "NuCache");
23+
Directory.CreateDirectory(path);
24+
25+
var localContentDbPath = Path.Combine(path, "NuCache.Content.db");
26+
var localContentDbExists = File.Exists(localContentDbPath);
27+
var contentDataSerializer = new ContentDataSerializer(new DictionaryOfPropertyDataSerializer());
28+
var localContentDb = BTree.GetTree(localContentDbPath, localContentDbExists, new NuCacheSettings(), contentDataSerializer);
29+
30+
return new ContentStore(
31+
GetRequiredService<IPublishedSnapshotAccessor>(),
32+
GetRequiredService<IVariationContextAccessor>(),
33+
LoggerFactory.CreateLogger<ContentCacheTests>(),
34+
LoggerFactory,
35+
GetRequiredService<IPublishedModelFactory>(), // new NoopPublishedModelFactory
36+
localContentDb);
37+
}
38+
39+
private ContentNodeKit CreateContentNodeKit()
40+
{
41+
var contentData = new ContentDataBuilder()
42+
.WithName("Content 1")
43+
.WithProperties(new PropertyDataBuilder()
44+
.WithPropertyData("welcomeText", "Welcome")
45+
.WithPropertyData("welcomeText", "Welcome", "en-US")
46+
.WithPropertyData("welcomeText", "Willkommen", "de")
47+
.WithPropertyData("welcomeText", "Welkom", "nl")
48+
.WithPropertyData("welcomeText2", "Welcome")
49+
.WithPropertyData("welcomeText2", "Welcome", "en-US")
50+
.WithPropertyData("noprop", "xxx")
51+
.Build())
52+
.Build();
53+
54+
return ContentNodeKitBuilder.CreateWithContent(
55+
ContentType.Id,
56+
1,
57+
"-1,1",
58+
draftData: contentData,
59+
publishedData: contentData);
60+
}
61+
62+
[Test]
63+
public async Task SetLocked()
64+
{
65+
var contentStore = GetContentStore();
66+
67+
using (contentStore.GetScopedWriteLock(ScopeProvider))
68+
{
69+
var contentNodeKit = CreateContentNodeKit();
70+
71+
contentStore.SetLocked(contentNodeKit);
72+
73+
// Try running the same operation again in an async task
74+
await Task.Run(() => contentStore.SetLocked(contentNodeKit));
75+
}
76+
}
77+
}

0 commit comments

Comments
 (0)