Skip to content

Commit a2511ff

Browse files
bergmaniakjac
bergmania
and
kjac
committed
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 01873aa. * Revert "Added Cache Instructions lock, to avoid deadlocks" This reverts commit e3fca7c. * 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 <kja@umbraco.dk> (cherry picked from commit 2c23e67)
1 parent 8e837d3 commit a2511ff

File tree

10 files changed

+129
-38
lines changed

10 files changed

+129
-38
lines changed

‎src/Umbraco.Cms.Persistence.SqlServer/Services/SqlServerDistributedLockingMechanism.cs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,9 +134,10 @@ private void ObtainReadLock()
134134

135135
const string query = "SELECT value FROM umbracoLock WITH (REPEATABLEREAD) WHERE id=@id";
136136

137-
db.Execute("SET LOCK_TIMEOUT " + _timeout.TotalMilliseconds + ";");
137+
var lockTimeoutQuery = $"SET LOCK_TIMEOUT {_timeout.TotalMilliseconds}";
138138

139-
var i = db.ExecuteScalar<int?>(query, new { id = LockId });
139+
// execute the lock timeout query and the actual query in a single server roundtrip
140+
var i = db.ExecuteScalar<int?>($"{lockTimeoutQuery};{query}", new { id = LockId });
140141

141142
if (i == null)
142143
{
@@ -169,9 +170,10 @@ private void ObtainWriteLock()
169170
const string query =
170171
@"UPDATE umbracoLock WITH (REPEATABLEREAD) SET value = (CASE WHEN (value=1) THEN -1 ELSE 1 END) WHERE id=@id";
171172

172-
db.Execute("SET LOCK_TIMEOUT " + _timeout.TotalMilliseconds + ";");
173+
var lockTimeoutQuery = $"SET LOCK_TIMEOUT {_timeout.TotalMilliseconds}";
173174

174-
var i = db.Execute(query, new { id = LockId });
175+
// execute the lock timeout query and the actual query in a single server roundtrip
176+
var i = db.Execute($"{lockTimeoutQuery};{query}", new { id = LockId });
175177

176178
if (i == 0)
177179
{

‎src/Umbraco.Cms.Persistence.Sqlite/Services/SqliteDistributedLockingMechanism.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ private void ObtainWriteLock()
154154

155155
try
156156
{
157-
var i = command.ExecuteNonQuery();
157+
var i = db.ExecuteNonQuery(command);
158158

159159
if (i == 0)
160160
{

‎src/Umbraco.Core/Cache/ObjectCacheAppCache.cs‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ namespace Umbraco.Cms.Core.Cache;
99
/// </summary>
1010
public class ObjectCacheAppCache : IAppPolicyCache, IDisposable
1111
{
12+
private static readonly TimeSpan _readLockTimeout = TimeSpan.FromSeconds(5);
13+
private static readonly TimeSpan _writeLockTimeout = TimeSpan.FromSeconds(5);
14+
1215
private readonly ReaderWriterLockSlim _locker = new(LockRecursionPolicy.SupportsRecursion);
1316
private bool _disposedValue;
1417

@@ -33,7 +36,10 @@ public ObjectCacheAppCache() =>
3336
Lazy<object?>? result;
3437
try
3538
{
36-
_locker.EnterReadLock();
39+
if (_locker.TryEnterReadLock(_readLockTimeout) is false)
40+
{
41+
throw new TimeoutException("Timeout exceeded to the memory cache when getting item");
42+
}
3743
result = MemoryCache.Get(key) as Lazy<object?>; // null if key not found
3844
}
3945
finally
@@ -195,7 +201,10 @@ public virtual void Clear(string key)
195201
{
196202
try
197203
{
198-
_locker.EnterWriteLock();
204+
if (_locker.TryEnterWriteLock(_writeLockTimeout) is false)
205+
{
206+
throw new TimeoutException("Timeout exceeded to the memory cache when clearing item");
207+
}
199208
if (MemoryCache[key] == null)
200209
{
201210
return;
@@ -223,8 +232,10 @@ public virtual void ClearOfType(Type type)
223232
var isInterface = type.IsInterface;
224233
try
225234
{
226-
_locker.EnterWriteLock();
227-
235+
if (_locker.TryEnterWriteLock(_writeLockTimeout) is false)
236+
{
237+
throw new TimeoutException("Timeout exceeded to the memory cache when clearing by type");
238+
}
228239
// ToArray required to remove
229240
foreach (var key in MemoryCache
230241
.Where(x =>
@@ -259,7 +270,10 @@ public virtual void ClearOfType<T>()
259270
{
260271
try
261272
{
262-
_locker.EnterWriteLock();
273+
if (_locker.TryEnterWriteLock(_writeLockTimeout) is false)
274+
{
275+
throw new TimeoutException("Timeout exceeded to the memory cache when clearing by generic type");
276+
}
263277
Type typeOfT = typeof(T);
264278
var isInterface = typeOfT.IsInterface;
265279

@@ -296,7 +310,10 @@ public virtual void ClearOfType<T>(Func<string, T, bool> predicate)
296310
{
297311
try
298312
{
299-
_locker.EnterWriteLock();
313+
if (_locker.TryEnterWriteLock(_writeLockTimeout) is false)
314+
{
315+
throw new TimeoutException("Timeout exceeded to the memory cache when clearing generic type with predicate");
316+
}
300317
Type typeOfT = typeof(T);
301318
var isInterface = typeOfT.IsInterface;
302319

@@ -338,7 +355,10 @@ public virtual void ClearByKey(string keyStartsWith)
338355
{
339356
try
340357
{
341-
_locker.EnterWriteLock();
358+
if (_locker.TryEnterWriteLock(_writeLockTimeout) is false)
359+
{
360+
throw new TimeoutException("Timeout exceeded to the memory cache when clearing with prefix");
361+
}
342362

343363
// ToArray required to remove
344364
foreach (var key in MemoryCache
@@ -365,7 +385,10 @@ public virtual void ClearByRegex(string regex)
365385

366386
try
367387
{
368-
_locker.EnterWriteLock();
388+
if (_locker.TryEnterWriteLock(_writeLockTimeout) is false)
389+
{
390+
throw new TimeoutException("Timeout exceeded to the memory cach when clearing by regex");
391+
}
369392

370393
// ToArray required to remove
371394
foreach (var key in MemoryCache

‎src/Umbraco.Infrastructure/Persistence/IUmbracoDatabase.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.Data.Common;
12
using NPoco;
23
using Umbraco.Cms.Infrastructure.Migrations.Install;
34

@@ -33,4 +34,7 @@ public interface IUmbracoDatabase : IDatabase
3334
bool IsUmbracoInstalled();
3435

3536
DatabaseSchemaResult ValidateSchema();
37+
38+
/// <returns>The number of rows affected.</returns>
39+
int ExecuteNonQuery(DbCommand command) => command.ExecuteNonQuery();
3640
}

‎src/Umbraco.Infrastructure/Persistence/UmbracoDatabase.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,14 @@ public DatabaseSchemaResult ValidateSchema()
224224
return databaseSchemaValidationResult ?? new DatabaseSchemaResult();
225225
}
226226

227+
public int ExecuteNonQuery(DbCommand command)
228+
{
229+
OnExecutingCommand(command);
230+
var i = command.ExecuteNonQuery();
231+
OnExecutedCommand(command);
232+
return i;
233+
}
234+
227235
/// <summary>
228236
/// Returns true if Umbraco database tables are detected to be installed
229237
/// </summary>

‎src/Umbraco.Infrastructure/Scoping/Scope.cs‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,9 @@ internal class Scope : ICoreScope, IScope, Core.Scoping.IScope
2323
private readonly bool _autoComplete;
2424
private readonly CoreDebugSettings _coreDebugSettings;
2525

26-
private readonly object _dictionaryLocker;
2726
private readonly IEventAggregator _eventAggregator;
2827
private readonly IsolationLevel _isolationLevel;
29-
private readonly object _lockQueueLocker = new();
28+
private readonly object _locker = new();
3029
private readonly ILogger<Scope> _logger;
3130
private readonly MediaFileManager _mediaFileManager;
3231
private readonly RepositoryCacheMode _repositoryCacheMode;
@@ -87,7 +86,6 @@ private Scope(
8786
_scopeFileSystem = scopeFileSystems;
8887
_autoComplete = autoComplete;
8988
Detachable = detachable;
90-
_dictionaryLocker = new object();
9189

9290
#if DEBUG_SCOPES
9391
_scopeProvider.RegisterScope(this);
@@ -562,7 +560,7 @@ public void Dispose()
562560
DisposeLastScope();
563561
}
564562

565-
lock (_lockQueueLocker)
563+
lock (_locker)
566564
{
567565
_queuedLocks?.Clear();
568566
}
@@ -573,24 +571,24 @@ public void Dispose()
573571
public void EagerReadLock(params int[] lockIds) => EagerReadLockInner(InstanceId, null, lockIds);
574572

575573
/// <inheritdoc />
576-
public void ReadLock(params int[] lockIds) => LazyReadLockInner(InstanceId, lockIds);
574+
public void ReadLock(params int[] lockIds) => EagerReadLockInner(InstanceId, null, lockIds);
577575

578576
public void EagerReadLock(TimeSpan timeout, int lockId) =>
579577
EagerReadLockInner(InstanceId, timeout, lockId);
580578

581579
/// <inheritdoc />
582-
public void ReadLock(TimeSpan timeout, int lockId) => LazyReadLockInner(InstanceId, timeout, lockId);
580+
public void ReadLock(TimeSpan timeout, int lockId) => EagerReadLockInner(InstanceId, timeout, lockId);
583581

584582
public void EagerWriteLock(params int[] lockIds) => EagerWriteLockInner(InstanceId, null, lockIds);
585583

586584
/// <inheritdoc />
587-
public void WriteLock(params int[] lockIds) => LazyWriteLockInner(InstanceId, lockIds);
585+
public void WriteLock(params int[] lockIds) => EagerWriteLockInner(InstanceId, null, lockIds);
588586

589587
public void EagerWriteLock(TimeSpan timeout, int lockId) =>
590588
EagerWriteLockInner(InstanceId, timeout, lockId);
591589

592590
/// <inheritdoc />
593-
public void WriteLock(TimeSpan timeout, int lockId) => LazyWriteLockInner(InstanceId, timeout, lockId);
591+
public void WriteLock(TimeSpan timeout, int lockId) => EagerWriteLockInner(InstanceId, timeout, lockId);
594592

595593
/// <summary>
596594
/// Used for testing. Ensures and gets any queued read locks.
@@ -659,7 +657,7 @@ private void EnsureDbLocks()
659657
}
660658
else
661659
{
662-
lock (_lockQueueLocker)
660+
lock (_locker)
663661
{
664662
if (_queuedLocks?.Count > 0)
665663
{
@@ -970,7 +968,7 @@ private void ClearLocks(Guid instanceId)
970968
}
971969
else
972970
{
973-
lock (_dictionaryLocker)
971+
lock (_locker)
974972
{
975973
_readLocksDictionary?.Remove(instanceId);
976974
_writeLocksDictionary?.Remove(instanceId);
@@ -1045,7 +1043,7 @@ public void LazyWriteLockInner(Guid instanceId, TimeSpan timeout, int lockId)
10451043

10461044
private void LazyLockInner(DistributedLockType lockType, Guid instanceId, params int[] lockIds)
10471045
{
1048-
lock (_lockQueueLocker)
1046+
lock (_locker)
10491047
{
10501048
if (_queuedLocks == null)
10511049
{
@@ -1061,7 +1059,7 @@ private void LazyLockInner(DistributedLockType lockType, Guid instanceId, params
10611059

10621060
private void LazyLockInner(DistributedLockType lockType, Guid instanceId, TimeSpan timeout, int lockId)
10631061
{
1064-
lock (_lockQueueLocker)
1062+
lock (_locker)
10651063
{
10661064
if (_queuedLocks == null)
10671065
{
@@ -1088,7 +1086,7 @@ private void EagerReadLockInner(Guid instanceId, TimeSpan? timeout, params int[]
10881086
}
10891087
else
10901088
{
1091-
lock (_dictionaryLocker)
1089+
lock (_locker)
10921090
{
10931091
foreach (var lockId in lockIds)
10941092
{
@@ -1122,7 +1120,7 @@ private void EagerWriteLockInner(Guid instanceId, TimeSpan? timeout, params int[
11221120
}
11231121
else
11241122
{
1125-
lock (_dictionaryLocker)
1123+
lock (_locker)
11261124
{
11271125
foreach (var lockId in lockIds)
11281126
{

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ namespace Umbraco.Cms.Infrastructure.PublishedCache;
2727
/// </remarks>
2828
public class ContentStore
2929
{
30+
private static readonly TimeSpan _monitorTimeout = TimeSpan.FromSeconds(30);
31+
3032
// TODO: collection trigger (ok for now)
3133
// see SnapDictionary notes
3234
private const long CollectMinGenDelta = 8;
@@ -330,7 +332,12 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
330332
throw new InvalidOperationException("Recursive locks not allowed");
331333
}
332334

333-
Monitor.Enter(_wlocko, ref lockInfo.Taken);
335+
Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken);
336+
337+
if (Monitor.IsEntered(_wlocko) is false)
338+
{
339+
throw new TimeoutException("Could not enter monitor before timeout in content store");
340+
}
334341

335342
lock (_rlocko)
336343
{

‎src/Umbraco.PublishedCache.NuCache/Persistence/NuCacheContentService.cs‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,25 @@ public void Rebuild(
127127
{
128128
using (ICoreScope scope = ScopeProvider.CreateCoreScope(repositoryCacheMode: RepositoryCacheMode.Scoped))
129129
{
130-
scope.ReadLock(Constants.Locks.ContentTree);
131-
scope.ReadLock(Constants.Locks.MediaTree);
132-
scope.ReadLock(Constants.Locks.MemberTree);
130+
if (contentTypeIds is null && mediaTypeIds is null && memberTypeIds is null)
131+
{
132+
scope.ReadLock(Constants.Locks.ContentTree,Constants.Locks.MediaTree,Constants.Locks.MemberTree);
133+
}
134+
135+
if (contentTypeIds is not null && contentTypeIds.Any())
136+
{
137+
scope.ReadLock(Constants.Locks.ContentTree);
138+
}
139+
140+
if (mediaTypeIds is not null && mediaTypeIds.Any())
141+
{
142+
scope.ReadLock(Constants.Locks.MediaTree);
143+
}
144+
145+
if (memberTypeIds is not null && memberTypeIds.Any())
146+
{
147+
scope.ReadLock(Constants.Locks.MemberTree);
148+
}
133149

134150
_repository.Rebuild(contentTypeIds, mediaTypeIds, memberTypeIds);
135151

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ public class SnapDictionary<TKey, TValue>
99
where TValue : class
1010
where TKey : notnull
1111
{
12+
private static readonly TimeSpan _monitorTimeout = TimeSpan.FromSeconds(30);
13+
1214
// minGenDelta to be adjusted
1315
// we may want to throttle collects even if delta is reached
1416
// we may want to force collect if delta is not reached but very old
@@ -198,7 +200,12 @@ private void Lock(WriteLockInfo lockInfo, bool forceGen = false)
198200
throw new InvalidOperationException("Recursive locks not allowed");
199201
}
200202

201-
Monitor.Enter(_wlocko, ref lockInfo.Taken);
203+
Monitor.TryEnter(_wlocko, _monitorTimeout, ref lockInfo.Taken);
204+
205+
if (Monitor.IsEntered(_wlocko) is false)
206+
{
207+
throw new TimeoutException("Could not enter the monitor before timeout in SnapDictionary");
208+
}
202209

203210
lock (_rlocko)
204211
{

0 commit comments

Comments
 (0)