diff --git a/src/Umbraco.Core/Runtime/MainDom.cs b/src/Umbraco.Core/Runtime/MainDom.cs index d7bd407015..67c7be3d21 100644 --- a/src/Umbraco.Core/Runtime/MainDom.cs +++ b/src/Umbraco.Core/Runtime/MainDom.cs @@ -151,7 +151,7 @@ namespace Umbraco.Core.Runtime { _logger.Info("Cannot acquire (timeout)."); - // TODO: In previous versions we'd let a TimeoutException be thrown + // In previous versions we'd let a TimeoutException be thrown // and the appdomain would not start. We have the opportunity to allow it to // start without having MainDom? This would mean that it couldn't write // to nucache/examine and would only be ok if this was a super short lived appdomain. diff --git a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs index 86426d196c..00ba721bdb 100644 --- a/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs +++ b/src/Umbraco.Tests/Cache/SnapDictionaryTests.cs @@ -9,47 +9,6 @@ using Umbraco.Web.PublishedCache.NuCache.Snap; namespace Umbraco.Tests.Cache { - /// - /// Used for tests - /// - public static class SnapDictionaryExtensions - { - internal static void Set(this SnapDictionary d, TKey key, TValue value) - where TValue : class - { - using (d.GetScopedWriteLock(GetScopeProvider())) - { - d.SetLocked(key, value); - } - } - - internal static void Clear(this SnapDictionary d) - where TValue : class - { - using (d.GetScopedWriteLock(GetScopeProvider())) - { - d.ClearLocked(); - } - } - - internal static void Clear(this SnapDictionary d, TKey key) - where TValue : class - { - using (d.GetScopedWriteLock(GetScopeProvider())) - { - d.ClearLocked(key); - } - } - - private static IScopeProvider GetScopeProvider() - { - var scopeProvider = Mock.Of(); - Mock.Get(scopeProvider) - .Setup(x => x.Context).Returns(() => null); - return scopeProvider; - } - } - [TestFixture] public class SnapDictionaryTests { @@ -1184,4 +1143,45 @@ namespace Umbraco.Tests.Cache return scopeProvider; } } + + /// + /// Used for tests so that we don't have to wrap every Set/Clear call in locks + /// + public static class SnapDictionaryExtensions + { + internal static void Set(this SnapDictionary d, TKey key, TValue value) + where TValue : class + { + using (d.GetScopedWriteLock(GetScopeProvider())) + { + d.SetLocked(key, value); + } + } + + internal static void Clear(this SnapDictionary d) + where TValue : class + { + using (d.GetScopedWriteLock(GetScopeProvider())) + { + d.ClearLocked(); + } + } + + internal static void Clear(this SnapDictionary d, TKey key) + where TValue : class + { + using (d.GetScopedWriteLock(GetScopeProvider())) + { + d.ClearLocked(key); + } + } + + private static IScopeProvider GetScopeProvider() + { + var scopeProvider = Mock.Of(); + Mock.Get(scopeProvider) + .Setup(x => x.Context).Returns(() => null); + return scopeProvider; + } + } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 8ce299932c..c959d9f67a 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -157,40 +157,44 @@ namespace Umbraco.Web.PublishedCache.NuCache private void Release(WriteLockInfo lockInfo, bool commit = true) { - if (commit == false) + try { - lock(_rlocko) + if (commit == false) { - // see SnapDictionary - try { } - finally + lock (_rlocko) { - _nextGen = false; - _liveGen -= 1; + // see SnapDictionary + try { } + finally + { + _nextGen = false; + _liveGen -= 1; + } } - } - Rollback(_contentNodes); - RollbackRoot(); - Rollback(_contentTypesById); - Rollback(_contentTypesByAlias); - } - else if (_localDb != null && _wchanges != null) - { - foreach (var change in _wchanges) + Rollback(_contentNodes); + RollbackRoot(); + Rollback(_contentTypesById); + Rollback(_contentTypesByAlias); + } + else if (_localDb != null && _wchanges != null) { - if (change.Value.IsNull) - _localDb.TryRemove(change.Key, out ContentNodeKit unused); - else - _localDb[change.Key] = change.Value; + foreach (var change in _wchanges) + { + if (change.Value.IsNull) + _localDb.TryRemove(change.Key, out ContentNodeKit unused); + else + _localDb[change.Key] = change.Value; + } + _wchanges = null; + _localDb.Commit(); } - _wchanges = null; - _localDb.Commit(); } - - // TODO: Shouldn't this be in a finally block? - if (lockInfo.Taken) - Monitor.Exit(_wlocko); + finally + { + if (lockInfo.Taken) + Monitor.Exit(_wlocko); + } } private void RollbackRoot() @@ -256,10 +260,6 @@ namespace Umbraco.Web.PublishedCache.NuCache } finally { - _logger.Info("Releasing ContentStore..."); - - // TODO: I don't understand this, we would have already closed the BPlusTree store above?? - // I guess it's 'safe' and will just decrement the write lock counter? Release(lockInfo); } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshot.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshot.cs index bbb84fc650..3f5c1aa4d5 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshot.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshot.cs @@ -39,15 +39,7 @@ namespace Umbraco.Web.PublishedCache.NuCache public void Resync() { - // This is annoying since this can cause deadlocks. Since this will be cleared if something happens in the same - // thread after that calls Elements, it will re-lock the _storesLock but that might already be locked again - // and since we're most likely in a ContentStore write lock, the other thread is probably wanting that one too. - // no lock - published snapshots are single-thread - - // TODO: Instead of clearing, we could hold this value in another var in case the above call to GetElements() Or potentially - // a new TryGetElements() fails (since we might be shutting down). In that case we can use the old value. - _elements?.Dispose(); _elements = null; } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index c37e5731e4..aee588d567 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -670,15 +670,7 @@ namespace Umbraco.Web.PublishedCache.NuCache publishedChanged = publishedChanged2; } - // TODO: These resync's are a problem, they cause deadlocks because when this is called, it's generally called within a writelock - // and then we clear out the snapshot and then if there's some event handler that needs the content cache, it tries to re-get it - // which first tries locking on the _storesLock which may have already been acquired and in this case we deadlock because - // we're still holding the write lock. - // We resync at the end of a ScopedWriteLock - // BUT if we don't resync here then the current snapshot is out of date for any event handlers that wish to use the most up to date - // data... - // so we need to figure out how to deal with the _storesLock - + if (draftChanged || publishedChanged) ((PublishedSnapshot)CurrentPublishedSnapshot)?.Resync(); }