diff --git a/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs b/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs index 6baa2cf0ed..9da202b5aa 100644 --- a/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs +++ b/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs @@ -24,8 +24,13 @@ namespace Umbraco.Core.Cache protected abstract object GetEntry(string key); // read-write lock the underlying cache - protected abstract IDisposable ReadLock { get; } - protected abstract IDisposable WriteLock { get; } + //protected abstract IDisposable ReadLock { get; } + //protected abstract IDisposable WriteLock { get; } + + protected abstract void EnterReadLock(); + protected abstract void ExitReadLock(); + protected abstract void EnterWriteLock(); + protected abstract void ExitWriteLock(); protected string GetCacheKey(string key) { @@ -88,21 +93,31 @@ namespace Umbraco.Core.Cache public virtual void ClearAllCache() { - using (WriteLock) + try { + EnterWriteLock(); foreach (var entry in GetDictionaryEntries() .ToArray()) RemoveEntry((string) entry.Key); } + finally + { + ExitWriteLock(); + } } public virtual void ClearCacheItem(string key) { var cacheKey = GetCacheKey(key); - using (WriteLock) + try { + EnterWriteLock(); RemoveEntry(cacheKey); } + finally + { + ExitWriteLock(); + } } public virtual void ClearCacheObjectTypes(string typeName) @@ -110,15 +125,16 @@ namespace Umbraco.Core.Cache var type = TypeFinder.GetTypeByName(typeName); if (type == null) return; var isInterface = type.IsInterface; - using (WriteLock) + try { + EnterWriteLock(); foreach (var entry in GetDictionaryEntries() .Where(x => { // entry.Value is Lazy and not null, its value may be null // remove null values as well, does not hurt // get non-created as NonCreatedValue & exceptions as null - var value = GetSafeLazyValue((Lazy)x.Value, true); + var value = GetSafeLazyValue((Lazy) x.Value, true); // if T is an interface remove anything that implements that interface // otherwise remove exact types (not inherited types) @@ -127,14 +143,19 @@ namespace Umbraco.Core.Cache .ToArray()) RemoveEntry((string) entry.Key); } + finally + { + ExitWriteLock(); + } } public virtual void ClearCacheObjectTypes() { var typeOfT = typeof(T); var isInterface = typeOfT.IsInterface; - using (WriteLock) + try { + EnterWriteLock(); foreach (var entry in GetDictionaryEntries() .Where(x => { @@ -142,7 +163,7 @@ namespace Umbraco.Core.Cache // remove null values as well, does not hurt // compare on exact type, don't use "is" // get non-created as NonCreatedValue & exceptions as null - var value = GetSafeLazyValue((Lazy)x.Value, true); + var value = GetSafeLazyValue((Lazy) x.Value, true); // if T is an interface remove anything that implements that interface // otherwise remove exact types (not inherited types) @@ -151,6 +172,10 @@ namespace Umbraco.Core.Cache .ToArray()) RemoveEntry((string) entry.Key); } + finally + { + ExitWriteLock(); + } } public virtual void ClearCacheObjectTypes(Func predicate) @@ -158,8 +183,9 @@ namespace Umbraco.Core.Cache var typeOfT = typeof(T); var isInterface = typeOfT.IsInterface; var plen = CacheItemPrefix.Length + 1; - using (WriteLock) + try { + EnterWriteLock(); foreach (var entry in GetDictionaryEntries() .Where(x => { @@ -167,7 +193,7 @@ namespace Umbraco.Core.Cache // remove null values as well, does not hurt // compare on exact type, don't use "is" // get non-created as NonCreatedValue & exceptions as null - var value = GetSafeLazyValue((Lazy)x.Value, true); + var value = GetSafeLazyValue((Lazy) x.Value, true); if (value == null) return true; // if T is an interface remove anything that implements that interface @@ -178,30 +204,44 @@ namespace Umbraco.Core.Cache })) RemoveEntry((string) entry.Key); } + finally + { + ExitWriteLock(); + } } public virtual void ClearCacheByKeySearch(string keyStartsWith) { var plen = CacheItemPrefix.Length + 1; - using (WriteLock) + try { + EnterWriteLock(); foreach (var entry in GetDictionaryEntries() .Where(x => ((string)x.Key).Substring(plen).InvariantStartsWith(keyStartsWith)) .ToArray()) RemoveEntry((string) entry.Key); } + finally + { + ExitWriteLock(); + } } public virtual void ClearCacheByKeyExpression(string regexString) { var plen = CacheItemPrefix.Length + 1; - using (WriteLock) + try { + EnterWriteLock(); foreach (var entry in GetDictionaryEntries() .Where(x => Regex.IsMatch(((string)x.Key).Substring(plen), regexString)) .ToArray()) RemoveEntry((string) entry.Key); } + finally + { + ExitWriteLock(); + } } #endregion @@ -212,12 +252,18 @@ namespace Umbraco.Core.Cache { var plen = CacheItemPrefix.Length + 1; IEnumerable entries; - using (ReadLock) + try { + EnterReadLock(); entries = GetDictionaryEntries() .Where(x => ((string)x.Key).Substring(plen).InvariantStartsWith(keyStartsWith)) .ToArray(); // evaluate while locked } + finally + { + ExitReadLock(); + } + return entries .Select(x => GetSafeLazyValue((Lazy)x.Value)) // return exceptions as null .Where(x => x != null); // backward compat, don't store null values in the cache @@ -228,12 +274,17 @@ namespace Umbraco.Core.Cache const string prefix = CacheItemPrefix + "-"; var plen = prefix.Length; IEnumerable entries; - using (ReadLock) + try { + EnterReadLock(); entries = GetDictionaryEntries() .Where(x => Regex.IsMatch(((string)x.Key).Substring(plen), regexString)) .ToArray(); // evaluate while locked } + finally + { + ExitReadLock(); + } return entries .Select(x => GetSafeLazyValue((Lazy)x.Value)) // return exceptions as null .Where(x => x != null); // backward compat, don't store null values in the cache @@ -243,10 +294,15 @@ namespace Umbraco.Core.Cache { cacheKey = GetCacheKey(cacheKey); Lazy result; - using (ReadLock) + try { + EnterReadLock(); result = GetEntry(cacheKey) as Lazy; // null if key not found } + finally + { + ExitReadLock(); + } return result == null ? null : GetSafeLazyValue(result); // return exceptions as null } diff --git a/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs index 6f97651042..5554655d83 100644 --- a/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs @@ -79,24 +79,26 @@ namespace Umbraco.Core.Cache #region Lock - protected override IDisposable ReadLock + private bool _entered; + + protected override void EnterReadLock() => EnterWriteLock(); + + protected override void EnterWriteLock() { - // there's no difference between ReadLock and WriteLock here - get { return WriteLock; } + if (HasContextItems) + { + System.Threading.Monitor.Enter(ContextItems.SyncRoot, ref _entered); + } } - protected override IDisposable WriteLock - { - // NOTE - // could think about just overriding base.Locker to return a different - // object but then we'd create a ReaderWriterLockSlim per request, - // which is less efficient than just using a basic monitor lock. + protected override void ExitReadLock() => ExitWriteLock(); - get + protected override void ExitWriteLock() + { + if (_entered) { - return HasContextItems - ? (IDisposable) new MonitorLock(ContextItems.SyncRoot) - : new NoopLocker(); + _entered = false; + System.Threading.Monitor.Exit(ContextItems.SyncRoot); } } @@ -113,8 +115,9 @@ namespace Umbraco.Core.Cache Lazy result; - using (WriteLock) + try { + EnterWriteLock(); result = ContextItems[cacheKey] as Lazy; // null if key not found // cannot create value within the lock, so if result.IsValueCreated is false, just @@ -127,6 +130,10 @@ namespace Umbraco.Core.Cache ContextItems[cacheKey] = result; } } + finally + { + ExitWriteLock(); + } // 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/Cache/HttpRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs index ad46201c0c..068bf3605e 100644 --- a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs @@ -50,14 +50,26 @@ namespace Umbraco.Core.Cache #region Lock - protected override IDisposable ReadLock + protected override void EnterReadLock() { - get { return new ReadLock(_locker); } + _locker.EnterReadLock(); } - protected override IDisposable WriteLock + protected override void EnterWriteLock() { - get { return new WriteLock(_locker); } + _locker.EnterWriteLock();; + } + + protected override void ExitReadLock() + { + if (_locker.IsReadLockHeld) + _locker.ExitReadLock(); + } + + protected override void ExitWriteLock() + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); } #endregion @@ -118,10 +130,16 @@ namespace Umbraco.Core.Cache // reads. We first try with a normal ReadLock for maximum concurrency and take the penalty of // having to re-lock in case there's no value. Would need to benchmark to figure out whether // it's worth it, though... - using (new ReadLock(_locker)) + try { + _locker.EnterReadLock(); result = _cache.Get(cacheKey) as Lazy; // null if key not found } + finally + { + if (_locker.IsReadLockHeld) + _locker.ExitReadLock(); + } var value = result == null ? null : GetSafeLazyValue(result); if (value != null) return value; @@ -195,11 +213,17 @@ 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); - using (new WriteLock(_locker)) + try { + _locker.EnterWriteLock(); //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } public void InsertCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) diff --git a/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs index 70e37addb8..b3a643dbfb 100644 --- a/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs @@ -37,20 +37,32 @@ namespace Umbraco.Core.Cache public virtual void ClearAllCache() { - using (new WriteLock(_locker)) + try { + _locker.EnterWriteLock(); MemoryCache.DisposeIfDisposable(); MemoryCache = new MemoryCache("in-memory"); } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } public virtual void ClearCacheItem(string key) { - using (new WriteLock(_locker)) + try { + _locker.EnterWriteLock(); if (MemoryCache[key] == null) return; MemoryCache.Remove(key); } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } public virtual void ClearCacheObjectTypes(string typeName) @@ -58,8 +70,9 @@ namespace Umbraco.Core.Cache var type = TypeFinder.GetTypeByName(typeName); if (type == null) return; var isInterface = type.IsInterface; - using (new WriteLock(_locker)) + try { + _locker.EnterWriteLock(); foreach (var key in MemoryCache .Where(x => { @@ -76,12 +89,18 @@ namespace Umbraco.Core.Cache .ToArray()) // ToArray required to remove MemoryCache.Remove(key); } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } public virtual void ClearCacheObjectTypes() { - using (new WriteLock(_locker)) + try { + _locker.EnterWriteLock(); var typeOfT = typeof (T); var isInterface = typeOfT.IsInterface; foreach (var key in MemoryCache @@ -101,12 +120,18 @@ namespace Umbraco.Core.Cache .ToArray()) // ToArray required to remove MemoryCache.Remove(key); } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } public virtual void ClearCacheObjectTypes(Func predicate) { - using (new WriteLock(_locker)) + try { + _locker.EnterWriteLock(); var typeOfT = typeof(T); var isInterface = typeOfT.IsInterface; foreach (var key in MemoryCache @@ -127,30 +152,47 @@ namespace Umbraco.Core.Cache .ToArray()) // ToArray required to remove MemoryCache.Remove(key); } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } public virtual void ClearCacheByKeySearch(string keyStartsWith) { - using (new WriteLock(_locker)) + try { + _locker.EnterWriteLock(); foreach (var key in MemoryCache .Where(x => x.Key.InvariantStartsWith(keyStartsWith)) .Select(x => x.Key) .ToArray()) // ToArray required to remove MemoryCache.Remove(key); } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } public virtual void ClearCacheByKeyExpression(string regexString) { - using (new WriteLock(_locker)) + try { + _locker.EnterWriteLock(); foreach (var key in MemoryCache .Where(x => Regex.IsMatch(x.Key, regexString)) .Select(x => x.Key) .ToArray()) // ToArray required to remove MemoryCache.Remove(key); } + finally + { + if (_locker.IsWriteLockHeld) + _locker.ExitWriteLock(); + } } #endregion @@ -160,12 +202,18 @@ namespace Umbraco.Core.Cache public IEnumerable GetCacheItemsByKeySearch(string keyStartsWith) { KeyValuePair[] entries; - using (new ReadLock(_locker)) + try { + _locker.EnterReadLock(); entries = MemoryCache .Where(x => x.Key.InvariantStartsWith(keyStartsWith)) .ToArray(); // evaluate while locked } + finally + { + if (_locker.IsReadLockHeld) + _locker.ExitReadLock(); + } return entries .Select(x => DictionaryCacheProviderBase.GetSafeLazyValue((Lazy)x.Value)) // return exceptions as null .Where(x => x != null) // backward compat, don't store null values in the cache @@ -175,12 +223,18 @@ namespace Umbraco.Core.Cache public IEnumerable GetCacheItemsByKeyExpression(string regexString) { KeyValuePair[] entries; - using (new ReadLock(_locker)) + try { + _locker.EnterReadLock(); entries = MemoryCache .Where(x => Regex.IsMatch(x.Key, regexString)) .ToArray(); // evaluate while locked } + finally + { + if (_locker.IsReadLockHeld) + _locker.ExitReadLock(); + } return entries .Select(x => DictionaryCacheProviderBase.GetSafeLazyValue((Lazy)x.Value)) // return exceptions as null .Where(x => x != null) // backward compat, don't store null values in the cache @@ -190,10 +244,16 @@ namespace Umbraco.Core.Cache public object GetCacheItem(string cacheKey) { Lazy result; - using (new ReadLock(_locker)) + try { + _locker.EnterReadLock(); result = MemoryCache.Get(cacheKey) as Lazy; // null if key not found } + finally + { + if (_locker.IsReadLockHeld) + _locker.ExitReadLock(); + } return result == null ? null : DictionaryCacheProviderBase.GetSafeLazyValue(result); // return exceptions as null } diff --git a/src/Umbraco.Core/Collections/ConcurrentHashSet.cs b/src/Umbraco.Core/Collections/ConcurrentHashSet.cs index 39cb701e69..4cad6e9f15 100644 --- a/src/Umbraco.Core/Collections/ConcurrentHashSet.cs +++ b/src/Umbraco.Core/Collections/ConcurrentHashSet.cs @@ -50,10 +50,16 @@ namespace Umbraco.Core.Collections /// The object to remove from the .The is read-only. public bool Remove(T item) { - using (new WriteLock(_instanceLocker)) + try { + _instanceLocker.EnterWriteLock(); return _innerSet.Remove(item); } + finally + { + if (_instanceLocker.IsWriteLockHeld) + _instanceLocker.ExitWriteLock(); + } } @@ -86,10 +92,16 @@ namespace Umbraco.Core.Collections /// The object to add to the .The is read-only. public void Add(T item) { - using (new WriteLock(_instanceLocker)) + try { + _instanceLocker.EnterWriteLock(); _innerSet.Add(item); } + finally + { + if (_instanceLocker.IsWriteLockHeld) + _instanceLocker.ExitWriteLock(); + } } /// @@ -101,13 +113,20 @@ namespace Umbraco.Core.Collections { var clone = GetThreadSafeClone(); if (clone.Contains(item)) return false; - using (new WriteLock(_instanceLocker)) + try { + _instanceLocker.EnterWriteLock(); + //double check if (_innerSet.Contains(item)) return false; _innerSet.Add(item); return true; } + finally + { + if (_instanceLocker.IsWriteLockHeld) + _instanceLocker.ExitWriteLock(); + } } /// @@ -116,10 +135,16 @@ namespace Umbraco.Core.Collections /// The is read-only. public void Clear() { - using (new WriteLock(_instanceLocker)) + try { + _instanceLocker.EnterWriteLock(); _innerSet.Clear(); } + finally + { + if (_instanceLocker.IsWriteLockHeld) + _instanceLocker.ExitWriteLock(); + } } /// @@ -147,10 +172,16 @@ namespace Umbraco.Core.Collections private HashSet GetThreadSafeClone() { HashSet clone = null; - using (new WriteLock(_instanceLocker)) + try { + _instanceLocker.EnterWriteLock(); clone = new HashSet(_innerSet, _innerSet.Comparer); } + finally + { + if (_instanceLocker.IsWriteLockHeld) + _instanceLocker.ExitWriteLock(); + } return clone; } diff --git a/src/Umbraco.Core/Models/PropertyGroupCollection.cs b/src/Umbraco.Core/Models/PropertyGroupCollection.cs index abec6febcb..0083bdb502 100644 --- a/src/Umbraco.Core/Models/PropertyGroupCollection.cs +++ b/src/Umbraco.Core/Models/PropertyGroupCollection.cs @@ -67,8 +67,10 @@ namespace Umbraco.Core.Models internal new void Add(PropertyGroup item) { - using (new WriteLock(_addLocker)) + try { + _addLocker.EnterWriteLock(); + //Note this is done to ensure existig groups can be renamed if (item.HasIdentity && item.Id > 0) { @@ -102,6 +104,11 @@ namespace Umbraco.Core.Models OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item)); } + finally + { + if (_addLocker.IsWriteLockHeld) + _addLocker.ExitWriteLock(); + } } /// diff --git a/src/Umbraco.Core/Models/PropertyTypeCollection.cs b/src/Umbraco.Core/Models/PropertyTypeCollection.cs index 4788221854..dbe97248f9 100644 --- a/src/Umbraco.Core/Models/PropertyTypeCollection.cs +++ b/src/Umbraco.Core/Models/PropertyTypeCollection.cs @@ -80,8 +80,9 @@ namespace Umbraco.Core.Models item.IsPublishing = IsPublishing; // fixme redo this entirely!!! - using (new WriteLock(_addLocker)) + try { + _addLocker.EnterWriteLock(); var key = GetKeyForItem(item); if (key != null) { @@ -105,6 +106,11 @@ namespace Umbraco.Core.Models OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item)); } + finally + { + if (_addLocker.IsWriteLockHeld) + _addLocker.ExitWriteLock(); + } } /// diff --git a/src/Umbraco.Core/Persistence/UmbracoDatabaseFactory.cs b/src/Umbraco.Core/Persistence/UmbracoDatabaseFactory.cs index efe05e951d..2b82ec7fac 100644 --- a/src/Umbraco.Core/Persistence/UmbracoDatabaseFactory.cs +++ b/src/Umbraco.Core/Persistence/UmbracoDatabaseFactory.cs @@ -126,8 +126,10 @@ namespace Umbraco.Core.Persistence /// public void Configure(string connectionString, string providerName) { - using (new WriteLock(_lock)) + try { + _lock.EnterWriteLock(); + _logger.Debug("Configuring."); if (Configured) throw new InvalidOperationException("Already configured."); @@ -173,6 +175,11 @@ namespace Umbraco.Core.Persistence _logger.Debug("Configured."); Configured = true; } + finally + { + if (_lock.IsWriteLockHeld) + _lock.ExitWriteLock(); + } } /// diff --git a/src/Umbraco.Core/ReadLock.cs b/src/Umbraco.Core/ReadLock.cs index 61025721fc..6e6ecf14b8 100644 --- a/src/Umbraco.Core/ReadLock.cs +++ b/src/Umbraco.Core/ReadLock.cs @@ -10,9 +10,11 @@ namespace Umbraco.Core /// Provides a convenience methodology for implementing locked access to resources. /// /// - /// Intended as an infrastructure class. + /// Intended as an infrastructure class. + /// This is a very unefficient 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("stop using, allocates")] public class ReadLock : IDisposable { private readonly ReaderWriterLockSlim _rwLock; diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 260f185607..e8a5a3654b 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -394,6 +394,7 @@ + @@ -1264,7 +1265,6 @@ - diff --git a/src/Umbraco.Core/WriteLock.cs b/src/Umbraco.Core/WriteLock.cs index b6a957234b..6ddc7e0bb3 100644 --- a/src/Umbraco.Core/WriteLock.cs +++ b/src/Umbraco.Core/WriteLock.cs @@ -10,9 +10,11 @@ namespace Umbraco.Core /// Provides a convenience methodology for implementing locked access to resources. /// /// - /// Intended as an infrastructure class. + /// Intended as an infrastructure class. + /// This is a very unefficient 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("stop using, allocates")] public class WriteLock : IDisposable { private readonly ReaderWriterLockSlim _rwLock;