diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 81229af9e1..450dc2f283 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -20,6 +20,7 @@ namespace Umbraco.Web.PublishedCache.NuCache // this class is an extended version of SnapDictionary // most of the snapshots management code, etc is an exact copy // SnapDictionary has unit tests to ensure it all works correctly + // For locking information, see SnapDictionary private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor; private readonly IVariationContextAccessor _variationContextAccessor; @@ -79,11 +80,6 @@ namespace Umbraco.Web.PublishedCache.NuCache private readonly string _instanceId = Guid.NewGuid().ToString("N"); - private class ReadLockInfo - { - public bool Taken; - } - private class WriteLockInfo { public bool Taken; @@ -121,15 +117,10 @@ namespace Umbraco.Web.PublishedCache.NuCache private void Lock(WriteLockInfo lockInfo, bool forceGen = false) { - // TODO: We are in a deadlock here somehow?? - Monitor.Enter(_wlocko, ref lockInfo.Taken); - var rtaken = false; - try + lock(_rlocko) { - Monitor.Enter(_rlocko, ref rtaken); - // see SnapDictionary try { } finally @@ -146,26 +137,16 @@ namespace Umbraco.Web.PublishedCache.NuCache _nextGen = true; } } - } - finally - { - if (rtaken) Monitor.Exit(_rlocko); - } - } - - private void Lock(ReadLockInfo lockInfo) - { - Monitor.Enter(_rlocko, ref lockInfo.Taken); + } } private void Release(WriteLockInfo lockInfo, bool commit = true) { if (commit == false) { - var rtaken = false; - try + lock(_rlocko) { - Monitor.Enter(_rlocko, ref rtaken); + // see SnapDictionary try { } finally { @@ -173,10 +154,6 @@ namespace Umbraco.Web.PublishedCache.NuCache _liveGen -= 1; } } - finally - { - if (rtaken) Monitor.Exit(_rlocko); - } Rollback(_contentNodes); RollbackRoot(); @@ -207,11 +184,6 @@ namespace Umbraco.Web.PublishedCache.NuCache Monitor.Exit(_wlocko); } - private void Release(ReadLockInfo lockInfo) - { - if (lockInfo.Taken) Monitor.Exit(_rlocko); - } - private void RollbackRoot() { if (_root.Gen <= _liveGen) return; @@ -1248,13 +1220,8 @@ namespace Umbraco.Web.PublishedCache.NuCache public Snapshot CreateSnapshot() { - var lockInfo = new ReadLockInfo(); - try + lock(_rlocko) { - // TODO: This would be much simpler with just a lock(_rlocko) { } - // in this case I see no reason why we are using this syntax?! - Lock(lockInfo); - // if no next generation is required, and we already have one, // use it and create a new snapshot if (_nextGen == false && _genObj != null) @@ -1306,10 +1273,6 @@ namespace Umbraco.Web.PublishedCache.NuCache return snapshot; } - finally - { - Release(lockInfo); - } } public Snapshot LiveSnapshot => new Snapshot(this, _liveGen @@ -1427,18 +1390,17 @@ namespace Umbraco.Web.PublishedCache.NuCache } } - // TODO: This is never used? Should it be? - - public async Task WaitForPendingCollect() - { - Task task; - lock (_rlocko) - { - task = _collectTask; - } - if (task != null) - await task; - } + // TODO: This is never used? Should it be? Maybe move to TestHelper below? + //public async Task WaitForPendingCollect() + //{ + // Task task; + // lock (_rlocko) + // { + // task = _collectTask; + // } + // if (task != null) + // await task; + //} public long GenCount => _genObjs.Count; diff --git a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs index 9671949ff0..e0ad26eb81 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/SnapDictionary.cs @@ -22,6 +22,11 @@ namespace Umbraco.Web.PublishedCache.NuCache // 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 ConcurrentQueue _genObjs; private GenObj _genObj; @@ -71,16 +76,11 @@ namespace Umbraco.Web.PublishedCache.NuCache // are all ignored - Release is private and meant to be invoked with 'commit' being false only // only on the outermost lock (by SnapDictionaryWriter) - // using (...) {} for locking is prone to nasty leaks in case of weird exceptions - // such as thread-abort or out-of-memory, but let's not worry about it now + // side note - using (...) {} for locking is prone to nasty leaks in case of weird exceptions + // such as thread-abort or out-of-memory, which is why we've moved away from the old using wrapper we had on locking. private readonly string _instanceId = Guid.NewGuid().ToString("N"); - private class ReadLockInfo - { - public bool Taken; - } - private class WriteLockInfo { public bool Taken; @@ -121,18 +121,16 @@ namespace Umbraco.Web.PublishedCache.NuCache { Monitor.Enter(_wlocko, ref lockInfo.Taken); - var rtaken = false; - try + lock(_rlocko) { - Monitor.Enter(_rlocko, ref rtaken); - // assume everything in finally runs atomically // http://stackoverflow.com/questions/18501678/can-this-unexpected-behavior-of-prepareconstrainedregions-and-thread-abort-be-ex // http://joeduffyblog.com/2005/03/18/atomicity-and-asynchronous-exception-failures/ // http://joeduffyblog.com/2007/02/07/introducing-the-new-readerwriterlockslim-in-orcas/ // http://chabster.blogspot.fr/2013/12/readerwriterlockslim-fails-on-dual.html //RuntimeHelpers.PrepareConstrainedRegions(); - try { } finally + try { } + finally { // increment the lock count, and register that this lock is counting _wlocked++; @@ -149,15 +147,6 @@ namespace Umbraco.Web.PublishedCache.NuCache } } } - finally - { - if (rtaken) Monitor.Exit(_rlocko); - } - } - - private void Lock(ReadLockInfo lockInfo) - { - Monitor.Enter(_rlocko, ref lockInfo.Taken); } private void Release(WriteLockInfo lockInfo, bool commit = true) @@ -168,22 +157,17 @@ namespace Umbraco.Web.PublishedCache.NuCache if (commit == false) { - var rtaken = false; - try + lock(_rlocko) { - Monitor.Enter(_rlocko, ref rtaken); - try { } finally + try { } + finally { // forget about the temp. liveGen _nextGen = false; _liveGen -= 1; } } - finally - { - if (rtaken) Monitor.Exit(_rlocko); - } - + foreach (var item in _items) { var link = item.Value; @@ -202,11 +186,6 @@ namespace Umbraco.Web.PublishedCache.NuCache Monitor.Exit(_wlocko); } - private void Release(ReadLockInfo lockInfo) - { - if (lockInfo.Taken) Monitor.Exit(_rlocko); - } - #endregion #region Set, Clear, Get, Has @@ -347,11 +326,8 @@ namespace Umbraco.Web.PublishedCache.NuCache public Snapshot CreateSnapshot() { - var lockInfo = new ReadLockInfo(); - try + lock(_rlocko) { - Lock(lockInfo); - // if no next generation is required, and we already have a gen object, // use it to create a new snapshot if (_nextGen == false && _genObj != null) @@ -398,10 +374,6 @@ namespace Umbraco.Web.PublishedCache.NuCache return snapshot; } - finally - { - Release(lockInfo); - } } public Task CollectAsync()