From e775497c5ef07b5c2da8f364d2a699c9c54a65d7 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 20 Apr 2021 16:09:52 +1000 Subject: [PATCH 1/5] Removes ReaderWriterLockSlim from PropertyGroupCollection and PropertyTypeCollection. No lock should exist here at all not even sure why it's there (Based on 9yr old code). I'm pretty sure it's there because these entities used to be cached (and not cloned) which meant it was the same instance used between threads. --- .../Models/PropertyGroupCollection.cs | 57 +++++++------------ .../Models/PropertyTypeCollection.cs | 51 +++++++---------- 2 files changed, 41 insertions(+), 67 deletions(-) diff --git a/src/Umbraco.Core/Models/PropertyGroupCollection.cs b/src/Umbraco.Core/Models/PropertyGroupCollection.cs index 973147b3fb..e2af5835f2 100644 --- a/src/Umbraco.Core/Models/PropertyGroupCollection.cs +++ b/src/Umbraco.Core/Models/PropertyGroupCollection.cs @@ -4,7 +4,6 @@ using System.Collections.ObjectModel; using System.Collections.Specialized; using System.Linq; using System.Runtime.Serialization; -using System.Threading; namespace Umbraco.Core.Models { @@ -17,8 +16,6 @@ namespace Umbraco.Core.Models // TODO: Change this to ObservableDictionary so we can reduce the INotifyCollectionChanged implementation details public class PropertyGroupCollection : KeyedCollection, INotifyCollectionChanged, IDeepCloneable { - private readonly ReaderWriterLockSlim _addLocker = new ReaderWriterLockSlim(); - internal PropertyGroupCollection() { } @@ -70,47 +67,37 @@ namespace Umbraco.Core.Models internal new void Add(PropertyGroup item) { - try + //Note this is done to ensure existing groups can be renamed + if (item.HasIdentity && item.Id > 0) { - _addLocker.EnterWriteLock(); - - //Note this is done to ensure existing groups can be renamed - if (item.HasIdentity && item.Id > 0) + var exists = Contains(item.Id); + if (exists) { - var exists = Contains(item.Id); + var keyExists = Contains(item.Name); + if (keyExists) + throw new Exception($"Naming conflict: Changing the name of PropertyGroup '{item.Name}' would result in duplicates"); + + //collection events will be raised in SetItem + SetItem(IndexOfKey(item.Id), item); + return; + } + } + else + { + var key = GetKeyForItem(item); + if (key != null) + { + var exists = Contains(key); if (exists) { - var keyExists = Contains(item.Name); - if (keyExists) - throw new Exception($"Naming conflict: Changing the name of PropertyGroup '{item.Name}' would result in duplicates"); - //collection events will be raised in SetItem - SetItem(IndexOfKey(item.Id), item); + SetItem(IndexOfKey(key), item); return; } } - else - { - var key = GetKeyForItem(item); - if (key != null) - { - var exists = Contains(key); - if (exists) - { - //collection events will be raised in SetItem - SetItem(IndexOfKey(key), item); - return; - } - } - } - //collection events will be raised in InsertItem - base.Add(item); - } - finally - { - if (_addLocker.IsWriteLockHeld) - _addLocker.ExitWriteLock(); } + //collection events will be raised in InsertItem + base.Add(item); } /// diff --git a/src/Umbraco.Core/Models/PropertyTypeCollection.cs b/src/Umbraco.Core/Models/PropertyTypeCollection.cs index cc9d00fa5d..b7a735e5dc 100644 --- a/src/Umbraco.Core/Models/PropertyTypeCollection.cs +++ b/src/Umbraco.Core/Models/PropertyTypeCollection.cs @@ -4,7 +4,6 @@ using System.Collections.ObjectModel; using System.Collections.Specialized; using System.Linq; using System.Runtime.Serialization; -using System.Threading; namespace Umbraco.Core.Models { @@ -16,10 +15,6 @@ namespace Umbraco.Core.Models // TODO: Change this to ObservableDictionary so we can reduce the INotifyCollectionChanged implementation details public class PropertyTypeCollection : KeyedCollection, INotifyCollectionChanged, IDeepCloneable { - [IgnoreDataMember] - private readonly ReaderWriterLockSlim _addLocker = new ReaderWriterLockSlim(); - - internal PropertyTypeCollection(bool supportsPublishing) { SupportsPublishing = supportsPublishing; @@ -87,36 +82,28 @@ namespace Umbraco.Core.Models item.SupportsPublishing = SupportsPublishing; // TODO: this is not pretty and should be refactored - try - { - _addLocker.EnterWriteLock(); - var key = GetKeyForItem(item); - if (key != null) - { - var exists = Contains(key); - if (exists) - { - //collection events will be raised in SetItem - SetItem(IndexOfKey(key), item); - return; - } - } - //check if the item's sort order is already in use - if (this.Any(x => x.SortOrder == item.SortOrder)) - { - //make it the next iteration - item.SortOrder = this.Max(x => x.SortOrder) + 1; - } - - //collection events will be raised in InsertItem - base.Add(item); - } - finally + var key = GetKeyForItem(item); + if (key != null) { - if (_addLocker.IsWriteLockHeld) - _addLocker.ExitWriteLock(); + var exists = Contains(key); + if (exists) + { + //collection events will be raised in SetItem + SetItem(IndexOfKey(key), item); + return; + } } + + //check if the item's sort order is already in use + if (this.Any(x => x.SortOrder == item.SortOrder)) + { + //make it the next iteration + item.SortOrder = this.Max(x => x.SortOrder) + 1; + } + + //collection events will be raised in InsertItem + base.Add(item); } /// From b75a7865519bd27f311ab7bba9746864d969dd32 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 20 Apr 2021 16:41:11 +1000 Subject: [PATCH 2/5] Ensures the ReaderWriterLockSlim within our caches are disposed at the end of the app --- src/Umbraco.Core/Cache/AppCaches.cs | 25 ++++++++++++++++- .../Cache/AppPolicedCacheDictionary.cs | 27 +++++++++++++++++-- src/Umbraco.Core/Cache/DeepCloneAppCache.cs | 23 +++++++++++++++- src/Umbraco.Core/Cache/ObjectCacheAppCache.cs | 21 ++++++++++++++- src/Umbraco.Core/Cache/WebCachingAppCache.cs | 22 ++++++++++++++- 5 files changed, 112 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Core/Cache/AppCaches.cs b/src/Umbraco.Core/Cache/AppCaches.cs index fbfc4c8c82..05453663f5 100644 --- a/src/Umbraco.Core/Cache/AppCaches.cs +++ b/src/Umbraco.Core/Cache/AppCaches.cs @@ -6,8 +6,10 @@ namespace Umbraco.Core.Cache /// /// Represents the application caches. /// - public class AppCaches + public class AppCaches : IDisposable { + private bool _disposedValue; + /// /// Initializes a new instance of the for use in a web application. /// @@ -82,5 +84,26 @@ namespace Umbraco.Core.Cache /// search through all keys on a global scale. /// public IsolatedCaches IsolatedCaches { get; } + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + RuntimeCache.DisposeIfDisposable(); + RequestCache.DisposeIfDisposable(); + IsolatedCaches.Dispose(); + } + + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } } } diff --git a/src/Umbraco.Core/Cache/AppPolicedCacheDictionary.cs b/src/Umbraco.Core/Cache/AppPolicedCacheDictionary.cs index 5c60dededa..4a6c032e4f 100644 --- a/src/Umbraco.Core/Cache/AppPolicedCacheDictionary.cs +++ b/src/Umbraco.Core/Cache/AppPolicedCacheDictionary.cs @@ -7,7 +7,7 @@ namespace Umbraco.Core.Cache /// Provides a base class for implementing a dictionary of . /// /// The type of the dictionary key. - public abstract class AppPolicedCacheDictionary + public abstract class AppPolicedCacheDictionary : IDisposable { private readonly ConcurrentDictionary _caches = new ConcurrentDictionary(); @@ -24,6 +24,7 @@ namespace Umbraco.Core.Cache /// Gets the internal cache factory, for tests only! /// internal readonly Func CacheFactory; + private bool _disposedValue; /// /// Gets or creates a cache. @@ -70,5 +71,27 @@ namespace Umbraco.Core.Cache foreach (var cache in _caches.Values) cache.Clear(); } + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + foreach(var value in _caches.Values) + { + value.DisposeIfDisposable(); + } + } + + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } } -} \ No newline at end of file +} diff --git a/src/Umbraco.Core/Cache/DeepCloneAppCache.cs b/src/Umbraco.Core/Cache/DeepCloneAppCache.cs index eff06e2aad..10a83d7423 100644 --- a/src/Umbraco.Core/Cache/DeepCloneAppCache.cs +++ b/src/Umbraco.Core/Cache/DeepCloneAppCache.cs @@ -12,8 +12,10 @@ namespace Umbraco.Core.Cache /// instance, and ensuring that all inserts and returns are deep cloned copies of the cache item, /// when the item is deep-cloneable. /// - internal class DeepCloneAppCache : IAppPolicyCache + internal class DeepCloneAppCache : IAppPolicyCache, IDisposable { + private bool _disposedValue; + /// /// Initializes a new instance of the class. /// @@ -153,5 +155,24 @@ namespace Umbraco.Core.Cache return input; } + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + InnerCache.DisposeIfDisposable(); + } + + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } } } diff --git a/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs b/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs index 5c4f76f51d..fd1e7f21f1 100644 --- a/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs +++ b/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs @@ -13,9 +13,10 @@ namespace Umbraco.Core.Cache /// /// Implements on top of a . /// - public class ObjectCacheAppCache : IAppPolicyCache + public class ObjectCacheAppCache : IAppPolicyCache, IDisposable { private readonly ReaderWriterLockSlim _locker = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion); + private bool _disposedValue; /// /// Initializes a new instance of the . @@ -360,5 +361,23 @@ namespace Umbraco.Core.Cache } return policy; } + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + _locker.Dispose(); + } + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } } } diff --git a/src/Umbraco.Core/Cache/WebCachingAppCache.cs b/src/Umbraco.Core/Cache/WebCachingAppCache.cs index bec596b129..d7377a193a 100644 --- a/src/Umbraco.Core/Cache/WebCachingAppCache.cs +++ b/src/Umbraco.Core/Cache/WebCachingAppCache.cs @@ -11,7 +11,7 @@ namespace Umbraco.Core.Cache /// Implements on top of a . /// /// The underlying cache is expected to be HttpRuntime.Cache. - internal class WebCachingAppCache : FastDictionaryAppCacheBase, IAppPolicyCache + internal class WebCachingAppCache : FastDictionaryAppCacheBase, IAppPolicyCache, IDisposable { // locker object that supports upgradeable read locking // does not need to support recursion if we implement the cache correctly and ensure @@ -19,6 +19,7 @@ namespace Umbraco.Core.Cache private readonly ReaderWriterLockSlim _locker = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion); private readonly System.Web.Caching.Cache _cache; + private bool _disposedValue; /// /// Initializes a new instance of the class. @@ -204,5 +205,24 @@ namespace Umbraco.Core.Cache _locker.ExitWriteLock(); } } + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + _locker.Dispose(); + } + + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } } } From 7e332b23c9852a827f8ba7691ed2fc3161d0d07c Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 20 Apr 2021 17:14:34 +1000 Subject: [PATCH 3/5] Fixes ConcurrentHashSet to just use a ConcurrentDictionary as it's underlying source and remove the ReaderWriterLockSlim since we are never disposing this. Makes IdkMap disposable. --- .../Collections/ConcurrentHashSet.cs | 109 +++--------------- src/Umbraco.Core/Services/IdkMap.cs | 29 ++++- 2 files changed, 41 insertions(+), 97 deletions(-) diff --git a/src/Umbraco.Core/Collections/ConcurrentHashSet.cs b/src/Umbraco.Core/Collections/ConcurrentHashSet.cs index c4dba51acd..0424c4dae1 100644 --- a/src/Umbraco.Core/Collections/ConcurrentHashSet.cs +++ b/src/Umbraco.Core/Collections/ConcurrentHashSet.cs @@ -1,8 +1,8 @@ using System; using System.Collections; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; -using System.Threading; namespace Umbraco.Core.Collections { @@ -14,9 +14,10 @@ namespace Umbraco.Core.Collections [Serializable] public class ConcurrentHashSet : ICollection { - private readonly HashSet _innerSet = new HashSet(); - private readonly ReaderWriterLockSlim _instanceLocker = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion); - + // Internally we just use a ConcurrentDictionary with the same value for the Value (since we don't actually care about the value) + private static readonly byte _emptyValue = 0x0; + private readonly ConcurrentDictionary _innerSet = new ConcurrentDictionary(); + /// /// Returns an enumerator that iterates through the collection. /// @@ -26,7 +27,7 @@ namespace Umbraco.Core.Collections /// 1 public IEnumerator GetEnumerator() { - return GetThreadSafeClone().GetEnumerator(); + return _innerSet.Keys.GetEnumerator(); } /// @@ -50,16 +51,7 @@ namespace Umbraco.Core.Collections /// The object to remove from the .The is read-only. public bool Remove(T item) { - try - { - _instanceLocker.EnterWriteLock(); - return _innerSet.Remove(item); - } - finally - { - if (_instanceLocker.IsWriteLockHeld) - _instanceLocker.ExitWriteLock(); - } + return _innerSet.TryRemove(item, out _); } @@ -74,17 +66,7 @@ namespace Umbraco.Core.Collections { get { - try - { - _instanceLocker.EnterReadLock(); - return _innerSet.Count; - } - finally - { - if (_instanceLocker.IsReadLockHeld) - _instanceLocker.ExitReadLock(); - } - + return _innerSet.Count; } } @@ -99,19 +81,10 @@ namespace Umbraco.Core.Collections /// /// Adds an item to the . /// - /// The object to add to the .The is read-only. + /// The object to add to the . public void Add(T item) { - try - { - _instanceLocker.EnterWriteLock(); - _innerSet.Add(item); - } - finally - { - if (_instanceLocker.IsWriteLockHeld) - _instanceLocker.ExitWriteLock(); - } + _innerSet.TryAdd(item, _emptyValue); } /// @@ -121,21 +94,7 @@ namespace Umbraco.Core.Collections /// public bool TryAdd(T item) { - if (Contains(item)) return false; - try - { - _instanceLocker.EnterWriteLock(); - - //double check - if (_innerSet.Contains(item)) return false; - _innerSet.Add(item); - return true; - } - finally - { - if (_instanceLocker.IsWriteLockHeld) - _instanceLocker.ExitWriteLock(); - } + return _innerSet.TryAdd(item, _emptyValue); } /// @@ -144,16 +103,7 @@ namespace Umbraco.Core.Collections /// The is read-only. public void Clear() { - try - { - _instanceLocker.EnterWriteLock(); - _innerSet.Clear(); - } - finally - { - if (_instanceLocker.IsWriteLockHeld) - _instanceLocker.ExitWriteLock(); - } + _innerSet.Clear(); } /// @@ -165,16 +115,7 @@ namespace Umbraco.Core.Collections /// The object to locate in the . public bool Contains(T item) { - try - { - _instanceLocker.EnterReadLock(); - return _innerSet.Contains(item); - } - finally - { - if (_instanceLocker.IsReadLockHeld) - _instanceLocker.ExitReadLock(); - } + return _innerSet.ContainsKey(item); } /// @@ -183,24 +124,8 @@ namespace Umbraco.Core.Collections /// The one-dimensional that is the destination of the elements copied from the . The array must have zero-based indexing.The zero-based index in at which copying begins. is a null reference (Nothing in Visual Basic). is less than zero. is equal to or greater than the length of the -or- The number of elements in the source is greater than the available space from to the end of the destination . public void CopyTo(T[] array, int index) { - var clone = GetThreadSafeClone(); - clone.CopyTo(array, index); - } - - private HashSet GetThreadSafeClone() - { - HashSet clone = null; - try - { - _instanceLocker.EnterReadLock(); - clone = new HashSet(_innerSet, _innerSet.Comparer); - } - finally - { - if (_instanceLocker.IsReadLockHeld) - _instanceLocker.ExitReadLock(); - } - return clone; + var keys = _innerSet.Keys; + keys.CopyTo(array, index); } /// @@ -209,8 +134,8 @@ namespace Umbraco.Core.Collections /// The one-dimensional that is the destination of the elements copied from . The must have zero-based indexing. The zero-based index in at which copying begins. is null. is less than zero. is multidimensional.-or- The number of elements in the source is greater than the available space from to the end of the destination . The type of the source cannot be cast automatically to the type of the destination . 2 public void CopyTo(Array array, int index) { - var clone = GetThreadSafeClone(); - Array.Copy(clone.ToArray(), 0, array, index, clone.Count); + var keys = _innerSet.Keys; + Array.Copy(keys.ToArray(), 0, array, index, keys.Count); } } } diff --git a/src/Umbraco.Core/Services/IdkMap.cs b/src/Umbraco.Core/Services/IdkMap.cs index f352def49e..b1caa1ea59 100644 --- a/src/Umbraco.Core/Services/IdkMap.cs +++ b/src/Umbraco.Core/Services/IdkMap.cs @@ -7,7 +7,7 @@ using Umbraco.Core.Scoping; namespace Umbraco.Core.Services { - public class IdkMap + public class IdkMap : IDisposable { private readonly IScopeProvider _scopeProvider; private readonly ReaderWriterLockSlim _locker = new ReaderWriterLockSlim(); @@ -46,6 +46,7 @@ namespace Umbraco.Core.Services private readonly ConcurrentDictionary id2key, Func key2id)> _dictionary = new ConcurrentDictionary id2key, Func key2id)>(); + private bool _disposedValue; internal void SetMapper(UmbracoObjectTypes umbracoObjectType, Func id2key, Func key2id) { @@ -106,8 +107,8 @@ namespace Umbraco.Core.Services } finally { - if (_locker.IsReadLockHeld) - _locker.ExitReadLock(); + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); } } @@ -125,8 +126,8 @@ namespace Umbraco.Core.Services } finally { - if (_locker.IsReadLockHeld) - _locker.ExitReadLock(); + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); } } #endif @@ -369,5 +370,23 @@ namespace Umbraco.Core.Services public T Id { get; } } + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + _locker.Dispose(); + } + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } } } From bf7a3251d80b2855605c6cf9b7c1c1e73712349a Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 20 Apr 2021 17:56:55 +1000 Subject: [PATCH 4/5] Ensure more ReaderWriterLockSlim are disposed or converted --- .../PureLiveModelFactory.cs | 27 ++++++++++++-- .../Logging/WebProfilerProvider.cs | 22 ++++-------- .../PublishedCache/NuCache/MemberCache.cs | 22 +++++++++++- .../NuCache/PublishedSnapshot.cs | 1 + .../PublishedContentTypeCache.cs | 23 +++++++++++- src/Umbraco.Web/Routing/SiteDomainHelper.cs | 36 ++++++++++++++----- 6 files changed, 101 insertions(+), 30 deletions(-) diff --git a/src/Umbraco.ModelsBuilder.Embedded/PureLiveModelFactory.cs b/src/Umbraco.ModelsBuilder.Embedded/PureLiveModelFactory.cs index 8ef99383a4..5b5f54cdc8 100644 --- a/src/Umbraco.ModelsBuilder.Embedded/PureLiveModelFactory.cs +++ b/src/Umbraco.ModelsBuilder.Embedded/PureLiveModelFactory.cs @@ -21,7 +21,7 @@ using File = System.IO.File; namespace Umbraco.ModelsBuilder.Embedded { - internal class PureLiveModelFactory : ILivePublishedModelFactory2, IRegisteredObject + internal class PureLiveModelFactory : ILivePublishedModelFactory2, IRegisteredObject, IDisposable { private Assembly _modelsAssembly; private Infos _infos = new Infos { ModelInfos = null, ModelTypeMap = new Dictionary() }; @@ -33,6 +33,7 @@ namespace Umbraco.ModelsBuilder.Embedded private int _ver, _skipver; private readonly int _debugLevel; private BuildManager _theBuildManager; + private bool _disposedValue; private readonly Lazy _umbracoServices; // fixme: this is because of circular refs :( private UmbracoServices UmbracoServices => _umbracoServices.Value; @@ -677,11 +678,31 @@ namespace Umbraco.ModelsBuilder.Embedded public void Stop(bool immediate) { - _watcher.EnableRaisingEvents = false; - _watcher.Dispose(); + Dispose(); HostingEnvironment.UnregisterObject(this); } + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + _watcher.EnableRaisingEvents = false; + _watcher.Dispose(); + _locker.Dispose(); + } + + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } + #endregion } } diff --git a/src/Umbraco.Web/Logging/WebProfilerProvider.cs b/src/Umbraco.Web/Logging/WebProfilerProvider.cs index f38a606745..8dad5f5b96 100755 --- a/src/Umbraco.Web/Logging/WebProfilerProvider.cs +++ b/src/Umbraco.Web/Logging/WebProfilerProvider.cs @@ -16,7 +16,7 @@ namespace Umbraco.Web.Logging /// internal class WebProfilerProvider : AspNetRequestProvider { - private readonly ReaderWriterLockSlim _locker = new ReaderWriterLockSlim(); + private readonly object _locker = new object(); private MiniProfiler _startupProfiler; private int _first; private volatile BootPhase _bootPhase; @@ -39,8 +39,7 @@ namespace Umbraco.Web.Logging public void BeginBootRequest() { - _locker.EnterWriteLock(); - try + lock (_locker) { if (_bootPhase != BootPhase.Boot) throw new InvalidOperationException("Invalid boot phase."); @@ -48,28 +47,19 @@ namespace Umbraco.Web.Logging // assign the profiler to be the current MiniProfiler for the request // is's already active, starting and all - HttpContext.Current.Items[":mini-profiler:"] = _startupProfiler; - } - finally - { - _locker.ExitWriteLock(); + HttpContext.Current.Items[":mini-profiler:"] = _startupProfiler; } } public void EndBootRequest() { - _locker.EnterWriteLock(); - try + lock (_locker) { if (_bootPhase != BootPhase.BootRequest) - throw new InvalidOperationException("Invalid boot phase."); + throw new InvalidOperationException("Invalid boot phase."); _bootPhase = BootPhase.Booted; - _startupProfiler = null; - } - finally - { - _locker.ExitWriteLock(); + _startupProfiler = null; } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/MemberCache.cs b/src/Umbraco.Web/PublishedCache/NuCache/MemberCache.cs index 2e196f629e..f311a7f302 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/MemberCache.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/MemberCache.cs @@ -13,7 +13,7 @@ using Umbraco.Web.PublishedCache.NuCache.Navigable; namespace Umbraco.Web.PublishedCache.NuCache { - internal class MemberCache : IPublishedMemberCache, INavigableData + internal class MemberCache : IPublishedMemberCache, INavigableData, IDisposable { private readonly IPublishedSnapshotAccessor _publishedSnapshotAccessor; @@ -23,6 +23,7 @@ namespace Umbraco.Web.PublishedCache.NuCache private readonly IMemberService _memberService; private readonly PublishedContentTypeCache _contentTypeCache; private readonly bool _previewDefault; + private bool _disposedValue; public MemberCache(bool previewDefault, IAppCache snapshotCache, IMemberService memberService, PublishedContentTypeCache contentTypeCache, IPublishedSnapshotAccessor publishedSnapshotAccessor, IVariationContextAccessor variationContextAccessor, IEntityXmlSerializer entitySerializer) @@ -158,6 +159,25 @@ namespace Umbraco.Web.PublishedCache.NuCache return _contentTypeCache.Get(PublishedItemType.Member, alias); } + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + _contentTypeCache.Dispose(); + } + + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } + #endregion } } diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshot.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshot.cs index 3f5c1aa4d5..85bad38ac7 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshot.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshot.cs @@ -32,6 +32,7 @@ namespace Umbraco.Web.PublishedCache.NuCache { ContentCache.Dispose(); MediaCache.Dispose(); + MemberCache.Dispose(); } } diff --git a/src/Umbraco.Web/PublishedCache/PublishedContentTypeCache.cs b/src/Umbraco.Web/PublishedCache/PublishedContentTypeCache.cs index 0f6e9af6bd..c9db02f091 100644 --- a/src/Umbraco.Web/PublishedCache/PublishedContentTypeCache.cs +++ b/src/Umbraco.Web/PublishedCache/PublishedContentTypeCache.cs @@ -13,7 +13,7 @@ namespace Umbraco.Web.PublishedCache /// Represents a content type cache. /// /// This cache is not snapshotted, so it refreshes any time things change. - public class PublishedContentTypeCache + public class PublishedContentTypeCache : IDisposable { // NOTE: These are not concurrent dictionaries because all access is done within a lock private readonly Dictionary _typesByAlias = new Dictionary(); @@ -320,6 +320,8 @@ namespace Umbraco.Web.PublishedCache // for unit tests - changing the callback must reset the cache obviously // TODO: Why does this even exist? For testing you'd pass in a mocked service to get by id private Func _getPublishedContentTypeById; + private bool _disposedValue; + internal Func GetPublishedContentTypeById { get => _getPublishedContentTypeById; @@ -367,5 +369,24 @@ namespace Umbraco.Web.PublishedCache { return GetAliasKey(contentType.ItemType, contentType.Alias); } + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + _lock.Dispose(); + } + + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } } } diff --git a/src/Umbraco.Web/Routing/SiteDomainHelper.cs b/src/Umbraco.Web/Routing/SiteDomainHelper.cs index 6173dfb43c..26937b2899 100644 --- a/src/Umbraco.Web/Routing/SiteDomainHelper.cs +++ b/src/Umbraco.Web/Routing/SiteDomainHelper.cs @@ -4,13 +4,14 @@ using System.Linq; using System.Threading; using System.Text.RegularExpressions; using Umbraco.Core; +using System.ComponentModel; namespace Umbraco.Web.Routing { /// /// Provides utilities to handle site domains. /// - public class SiteDomainHelper : ISiteDomainHelper + public class SiteDomainHelper : ISiteDomainHelper, IDisposable { #region Configure @@ -18,6 +19,7 @@ namespace Umbraco.Web.Routing private static Dictionary _sites; private static Dictionary> _bindings; private static Dictionary> _qualifiedSites; + private bool _disposedValue; // these are for unit tests *only* // ReSharper disable ConvertToAutoPropertyWithPrivateSetter @@ -30,16 +32,10 @@ namespace Umbraco.Web.Routing private const string DomainValidationSource = @"^(((?i:http[s]?://)?([-\w]+(\.[-\w]+)*)(:\d+)?(/)?))$"; private static readonly Regex DomainValidation = new Regex(DomainValidationSource, RegexOptions.IgnoreCase | RegexOptions.Compiled); - /// - /// Returns a disposable object that represents safe write access to config. - /// - /// Should be used in a using(SiteDomainHelper.ConfigWriteLock) { ... } mode. + [EditorBrowsable(EditorBrowsableState.Never)] protected static IDisposable ConfigWriteLock => new WriteLock(ConfigLock); - /// - /// Returns a disposable object that represents safe read access to config. - /// - /// Should be used in a using(SiteDomainHelper.ConfigWriteLock) { ... } mode. + [EditorBrowsable(EditorBrowsableState.Never)] protected static IDisposable ConfigReadLock => new ReadLock(ConfigLock); /// @@ -313,6 +309,28 @@ namespace Umbraco.Web.Routing return ret; } + #endregion + + protected virtual void Dispose(bool disposing) + { + if (!_disposedValue) + { + if (disposing) + { + // This is pretty nasty disposing a static on an instance but it's because this whole class + // is pretty fubar. I'm sure we've fixed this all up in netcore now? We need to remove all statics. + ConfigLock.Dispose(); + } + + _disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + } } } From dc089970408355eb7a34d8e3901e036caa070fd5 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 20 Apr 2021 18:02:23 +1000 Subject: [PATCH 5/5] Removes usages of UpgradeableReadLock --- src/Umbraco.Core/Cache/ObjectCacheAppCache.cs | 23 +++++++++++++--- src/Umbraco.Core/Cache/WebCachingAppCache.cs | 27 ++++++++++++++----- src/Umbraco.Core/ReadLock.cs | 13 +-------- src/Umbraco.Core/UpgradeableReadLock.cs | 10 +------ src/Umbraco.Core/WriteLock.cs | 13 +-------- 5 files changed, 43 insertions(+), 43 deletions(-) diff --git a/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs b/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs index fd1e7f21f1..e2f6017608 100644 --- a/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs +++ b/src/Umbraco.Core/Cache/ObjectCacheAppCache.cs @@ -110,19 +110,34 @@ namespace Umbraco.Core.Cache Lazy result; - using (var lck = new UpgradeableReadLock(_locker)) + try { + _locker.EnterUpgradeableReadLock(); + result = MemoryCache.Get(key) as Lazy; if (result == null || FastDictionaryAppCacheBase.GetSafeLazyValue(result, true) == null) // get non-created as NonCreatedValue & exceptions as null { result = FastDictionaryAppCacheBase.GetSafeLazy(factory); var policy = GetPolicy(timeout, isSliding, removedCallback, dependentFiles); - lck.UpgradeToWriteLock(); - //NOTE: This does an add or update - MemoryCache.Set(key, result, policy); + try + { + _locker.EnterWriteLock(); + //NOTE: This does an add or update + MemoryCache.Set(key, result, policy); + } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } } + finally + { + if (_locker.IsUpgradeableReadLockHeld) + _locker.ExitUpgradeableReadLock(); + } //return result.Value; diff --git a/src/Umbraco.Core/Cache/WebCachingAppCache.cs b/src/Umbraco.Core/Cache/WebCachingAppCache.cs index d7377a193a..671cd2d9c4 100644 --- a/src/Umbraco.Core/Cache/WebCachingAppCache.cs +++ b/src/Umbraco.Core/Cache/WebCachingAppCache.cs @@ -139,8 +139,10 @@ namespace Umbraco.Core.Cache var value = result == null ? null : GetSafeLazyValue(result); if (value != null) return value; - using (var lck = new UpgradeableReadLock(_locker)) + try { + _locker.EnterUpgradeableReadLock(); + result = _cache.Get(key) as Lazy; // null if key not found // cannot create value within the lock, so if result.IsValueCreated is false, just @@ -153,15 +155,28 @@ namespace Umbraco.Core.Cache var absolute = isSliding ? System.Web.Caching.Cache.NoAbsoluteExpiration : (timeout == null ? System.Web.Caching.Cache.NoAbsoluteExpiration : DateTime.Now.Add(timeout.Value)); var sliding = isSliding == false ? System.Web.Caching.Cache.NoSlidingExpiration : (timeout ?? System.Web.Caching.Cache.NoSlidingExpiration); - lck.UpgradeToWriteLock(); + try + { + _locker.EnterWriteLock(); - // create a cache dependency if one is needed. - var dependency = dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null; + // create a cache dependency if one is needed. + var dependency = dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null; - //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! - _cache.Insert(key, result, dependency, absolute, sliding, priority, removedCallback); + //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! + _cache.Insert(key, result, dependency, absolute, sliding, priority, removedCallback); + } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } } + finally + { + if (_locker.IsUpgradeableReadLockHeld) + _locker.ExitUpgradeableReadLock(); + } // using GetSafeLazy and GetSafeLazyValue ensures that we don't cache // exceptions (but try again and again) and silently eat them - however at diff --git a/src/Umbraco.Core/ReadLock.cs b/src/Umbraco.Core/ReadLock.cs index 9d3ef22168..f830d08bfa 100644 --- a/src/Umbraco.Core/ReadLock.cs +++ b/src/Umbraco.Core/ReadLock.cs @@ -1,20 +1,9 @@ using System; -using System.Collections.Generic; -using System.Linq; -using System.Text; using System.Threading; namespace Umbraco.Core { - /// - /// Provides a convenience methodology for implementing locked access to resources. - /// - /// - /// Intended as an infrastructure class. - /// This is a very inefficient way to lock as it allocates one object each time we lock, - /// so it's OK to use this class for things that happen once, where it is convenient, but not - /// for performance-critical code! - /// + [Obsolete("Use ReaderWriterLockSlim directly. This will be removed in future versions.")] public class ReadLock : IDisposable { private readonly ReaderWriterLockSlim _rwLock; diff --git a/src/Umbraco.Core/UpgradeableReadLock.cs b/src/Umbraco.Core/UpgradeableReadLock.cs index e3717fdf9a..656470cd8a 100644 --- a/src/Umbraco.Core/UpgradeableReadLock.cs +++ b/src/Umbraco.Core/UpgradeableReadLock.cs @@ -1,17 +1,9 @@ using System; -using System.Collections.Generic; -using System.Linq; -using System.Text; using System.Threading; namespace Umbraco.Core { - /// - /// Provides a convenience methodology for implementing locked access to resources. - /// - /// - /// Intended as an infrastructure class. - /// + [Obsolete("Use ReaderWriterLockSlim directly. This will be removed in future versions.")] public class UpgradeableReadLock : IDisposable { private readonly ReaderWriterLockSlim _rwLock; diff --git a/src/Umbraco.Core/WriteLock.cs b/src/Umbraco.Core/WriteLock.cs index dfa170218b..1b698dc59c 100644 --- a/src/Umbraco.Core/WriteLock.cs +++ b/src/Umbraco.Core/WriteLock.cs @@ -1,20 +1,9 @@ using System; -using System.Collections.Generic; -using System.Linq; -using System.Text; using System.Threading; namespace Umbraco.Core { - /// - /// Provides a convenience methodology for implementing locked access to resources. - /// - /// - /// Intended as an infrastructure class. - /// This is a very inefficient way to lock as it allocates one object each time we lock, - /// so it's OK to use this class for things that happen once, where it is convenient, but not - /// for performance-critical code! - /// + [Obsolete("Use ReaderWriterLockSlim directly. This will be removed in future versions.")] public class WriteLock : IDisposable { private readonly ReaderWriterLockSlim _rwLock;