From 8729156fbf1b89bdadcf1820cea7caa02827b59e Mon Sep 17 00:00:00 2001 From: Stephan Date: Fri, 16 May 2014 16:48:31 +0200 Subject: [PATCH] U4-4931 - lock contention & potential application lockdown Conflicts: src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs src/umbraco.cms/Actions/Action.cs --- .../Cache/HttpRuntimeCacheProvider.cs | 42 ++++++++++++------- .../Cache/ObjectCacheRuntimeCacheProvider.cs | 33 ++++++++------- .../ObjectResolution/Resolution.cs | 5 +++ src/umbraco.cms/Actions/Action.cs | 23 +++++----- 4 files changed, 64 insertions(+), 39 deletions(-) diff --git a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs index c52d27f24a..46ecbf7c2b 100644 --- a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs @@ -81,25 +81,36 @@ namespace Umbraco.Core.Cache { cacheKey = GetCacheKey(cacheKey); + // NOTE - because we don't know what getCacheItem does, how long it will take and whether it will hang, + // getCacheItem should run OUTSIDE of the global application lock else we run into lock contention and + // nasty performance issues. + + // So.... we insert a Lazy in the cache while holding the global application lock, and then rely + // on the Lazy lock to ensure that getCacheItem runs once and everybody waits on it, while the global + // application lock has been released. + + // Note that this means we'll end up storing null values in the cache, whereas in the past we made sure + // not to store them. There's code below, commented out, to make sure we do re-compute the value if it + // was previously computed as null - effectively reproducing the past behavior - but I'm not quite sure + // it is a good idea. + + Lazy result; + using (var lck = new UpgradeableReadLock(Locker)) { - var result = DictionaryCache.Get(cacheKey); - if (result == null) + result = DictionaryCache.Get(cacheKey) as Lazy; + if (result == null /* || (result.IsValueCreated && result.Value == null) */) { lck.UpgradeToWriteLock(); - result = getCacheItem(); - if (result != null) - { - 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); - - _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); - } - + result = new Lazy(getCacheItem); + 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); + _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); } - return result; } + + return result.Value; } public object GetCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) @@ -124,8 +135,11 @@ namespace Umbraco.Core.Cache /// internal void InsertCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, CacheDependency dependency = null) { - var result = getCacheItem(); - if (result == null) return; + // NOTE - here also we must insert a Lazy but we can evaluate it right now + // and make sure we don't store a null value. Though I'm not sure it is a good idea. + + var result = new Lazy(getCacheItem); + //if (result.Value == null) return; cacheKey = GetCacheKey(cacheKey); diff --git a/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs index 7b1053c8a4..89a39c86d9 100644 --- a/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs @@ -141,32 +141,35 @@ namespace Umbraco.Core.Cache CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { + // see notes in HttpRuntimeCacheProvider + + Lazy result; + using (var lck = new UpgradeableReadLock(Locker)) { - var result = MemoryCache.Get(cacheKey); - if (result == null) + result = MemoryCache.Get(cacheKey) as Lazy; + if (result == null /* || (result.IsValueCreated && result.Value == null) */) { lck.UpgradeToWriteLock(); - result = getCacheItem(); - if (result != null) - { - var policy = GetPolicy(timeout, isSliding, removedCallback, dependentFiles); - MemoryCache.Set(cacheKey, result, policy); - } + result = new Lazy(getCacheItem); + var policy = GetPolicy(timeout, isSliding, removedCallback, dependentFiles); + MemoryCache.Set(cacheKey, result, policy); } - return result; } + + return result.Value; } public void InsertCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - object result = getCacheItem(); - if (result != null) - { - var policy = GetPolicy(timeout, isSliding, removedCallback, dependentFiles); - MemoryCache.Set(cacheKey, result, policy); - } + // see notes in HttpRuntimeCacheProvider + + var result = new Lazy(getCacheItem); + //if (result.Value == null) return; + + var policy = GetPolicy(timeout, isSliding, removedCallback, dependentFiles); + MemoryCache.Set(cacheKey, result, policy); } private static CacheItemPolicy GetPolicy(TimeSpan? timeout = null, bool isSliding = false, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) diff --git a/src/Umbraco.Core/ObjectResolution/Resolution.cs b/src/Umbraco.Core/ObjectResolution/Resolution.cs index 17097ee7a5..87eb06e295 100644 --- a/src/Umbraco.Core/ObjectResolution/Resolution.cs +++ b/src/Umbraco.Core/ObjectResolution/Resolution.cs @@ -63,6 +63,11 @@ namespace Umbraco.Core.ObjectResolution } } + // NOTE - the ugly code below exists only because of umbraco.BusinessLogic.Actions.Action.ReRegisterActionsAndHandlers + // which wants to re-register actions and handlers instead of properly restarting the application. Don't even think + // about using it for anything else. Also, while the backdoor is open, the resolution system is locked so nothing + // can work properly => deadlocks. Therefore, open the backdoor, do resolution changes EXCLUSIVELY, and close the door! + /// /// Returns a disposable object that reprents dirty access to temporarily unfrozen resolution configuration. /// diff --git a/src/umbraco.cms/Actions/Action.cs b/src/umbraco.cms/Actions/Action.cs index a4e7c097ce..075cd84436 100644 --- a/src/umbraco.cms/Actions/Action.cs +++ b/src/umbraco.cms/Actions/Action.cs @@ -33,7 +33,6 @@ namespace umbraco.BusinessLogic.Actions [Obsolete("Actions and ActionHandlers are obsolete and should no longer be used")] public class Action { - private static readonly Dictionary ActionJs = new Dictionary(); private static readonly object Lock = new object(); @@ -49,21 +48,25 @@ namespace umbraco.BusinessLogic.Actions /// /// /// TODO: this shouldn't be needed... we should restart the app pool when a package is installed! - /// + /// public static void ReRegisterActionsAndHandlers() { - lock (Lock) - { + lock (Lock) + { + // NOTE use the DirtyBackdoor to change the resolution configuration EXCLUSIVELY + // ie do NOT do ANYTHING else while holding the backdoor, because while it is open + // the whole resolution system is locked => nothing can work properly => deadlocks + + var newResolver = new ActionsResolver( + () => TypeFinder.FindClassesOfType(PluginManager.Current.AssembliesToScan)); + using (Umbraco.Core.ObjectResolution.Resolution.DirtyBackdoorToConfiguration) { - //TODO: Based on the above, this is a big hack as types should all be cleared on package install! ActionsResolver.Reset(false); // and do NOT reset the whole resolution! - - //TODO: Based on the above, this is a big hack as types should all be cleared on package install! - ActionsResolver.Current = new ActionsResolver( - () => TypeFinder.FindClassesOfType(PluginManager.Current.AssembliesToScan)); + ActionsResolver.Current = newResolver; } - } + + } } ///