diff --git a/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs b/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs index 5a1c9b62de..c4895f1fe2 100644 --- a/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs +++ b/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs @@ -3,7 +3,6 @@ using System.Collections; using System.Collections.Generic; using System.Linq; using System.Text.RegularExpressions; -using System.Threading; namespace Umbraco.Core.Cache { @@ -12,7 +11,8 @@ namespace Umbraco.Core.Cache // prefix cache keys so we know which one are ours protected const string CacheItemPrefix = "umbrtmche"; - protected readonly ReaderWriterLockSlim Locker = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion); + // an object that represent a value that has not been created yet + protected readonly object ValueNotCreated = new object(); // manupulate the underlying cache entries // these *must* be called from within the appropriate locks @@ -21,15 +21,22 @@ namespace Umbraco.Core.Cache protected abstract void RemoveEntry(string key); protected abstract object GetEntry(string key); + // read-write lock the underlying cache + protected abstract IDisposable ReadLock { get; } + protected abstract IDisposable WriteLock { get; } + protected string GetCacheKey(string key) { return string.Format("{0}-{1}", CacheItemPrefix, key); } - protected object GetSafeLazyValue(Lazy lazy) + protected object GetSafeLazyValue(Lazy lazy, bool onlyIfValueIsCreated = false) { try { + // if onlyIfValueIsCreated, do not trigger value creation + // must return something, though, to differenciate from null values + if (onlyIfValueIsCreated && lazy.IsValueCreated == false) return ValueNotCreated; return lazy.Value; } catch @@ -42,7 +49,7 @@ namespace Umbraco.Core.Cache public virtual void ClearAllCache() { - using (new WriteLock(Locker)) + using (WriteLock) { foreach (var entry in GetDictionaryEntries() .ToArray()) @@ -52,23 +59,24 @@ namespace Umbraco.Core.Cache public virtual void ClearCacheItem(string key) { - using (new WriteLock(Locker)) + var cacheKey = GetCacheKey(key); + using (WriteLock) { - var cacheKey = GetCacheKey(key); RemoveEntry(cacheKey); } } public virtual void ClearCacheObjectTypes(string typeName) { - using (new WriteLock(Locker)) + using (WriteLock) { 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 - var value = GetSafeLazyValue((Lazy) x.Value); // return exceptions as null + // get non-created as NonCreatedValue & exceptions as null + var value = GetSafeLazyValue((Lazy)x.Value, true); return value == null || value.GetType().ToString().InvariantEquals(typeName); }) .ToArray()) @@ -79,7 +87,7 @@ namespace Umbraco.Core.Cache public virtual void ClearCacheObjectTypes() { var typeOfT = typeof(T); - using (new WriteLock(Locker)) + using (WriteLock) { foreach (var entry in GetDictionaryEntries() .Where(x => @@ -87,7 +95,8 @@ namespace Umbraco.Core.Cache // entry.Value is Lazy and not null, its value may be null // remove null values as well, does not hurt // compare on exact type, don't use "is" - var value = GetSafeLazyValue((Lazy)x.Value); // return exceptions as null + // get non-created as NonCreatedValue & exceptions as null + var value = GetSafeLazyValue((Lazy)x.Value, true); return value == null || value.GetType() == typeOfT; }) .ToArray()) @@ -99,7 +108,7 @@ namespace Umbraco.Core.Cache { var typeOfT = typeof(T); var plen = CacheItemPrefix.Length + 1; - using (new WriteLock(Locker)) + using (WriteLock) { foreach (var entry in GetDictionaryEntries() .Where(x => @@ -107,7 +116,8 @@ namespace Umbraco.Core.Cache // entry.Value is Lazy and not null, its value may be null // remove null values as well, does not hurt // compare on exact type, don't use "is" - var value = GetSafeLazyValue((Lazy)x.Value); // return exceptions as null + // get non-created as NonCreatedValue & exceptions as null + var value = GetSafeLazyValue((Lazy)x.Value, true); if (value == null) return true; return value.GetType() == typeOfT // run predicate on the 'public key' part only, ie without prefix @@ -120,7 +130,7 @@ namespace Umbraco.Core.Cache public virtual void ClearCacheByKeySearch(string keyStartsWith) { var plen = CacheItemPrefix.Length + 1; - using (new WriteLock(Locker)) + using (WriteLock) { foreach (var entry in GetDictionaryEntries() .Where(x => ((string)x.Key).Substring(plen).InvariantStartsWith(keyStartsWith)) @@ -132,7 +142,7 @@ namespace Umbraco.Core.Cache public virtual void ClearCacheByKeyExpression(string regexString) { var plen = CacheItemPrefix.Length + 1; - using (new WriteLock(Locker)) + using (WriteLock) { foreach (var entry in GetDictionaryEntries() .Where(x => Regex.IsMatch(((string)x.Key).Substring(plen), regexString)) @@ -148,38 +158,43 @@ namespace Umbraco.Core.Cache public virtual IEnumerable GetCacheItemsByKeySearch(string keyStartsWith) { var plen = CacheItemPrefix.Length + 1; - using (new ReadLock(Locker)) + IEnumerable entries; + using (ReadLock) { - return GetDictionaryEntries() + entries = GetDictionaryEntries() .Where(x => ((string)x.Key).Substring(plen).InvariantStartsWith(keyStartsWith)) - .Select(x => GetSafeLazyValue((Lazy)x.Value)) // return exceptions as null - .Where(x => x != null) // backward compat, don't store null values in the cache - .ToList(); + .ToArray(); // evaluate while locked } + 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 } public virtual IEnumerable GetCacheItemsByKeyExpression(string regexString) { const string prefix = CacheItemPrefix + "-"; var plen = prefix.Length; - using (new ReadLock(Locker)) + IEnumerable entries; + using (ReadLock) { - return GetDictionaryEntries() + entries = GetDictionaryEntries() .Where(x => Regex.IsMatch(((string)x.Key).Substring(plen), regexString)) - .Select(x => GetSafeLazyValue((Lazy)x.Value)) // return exceptions as null - .Where(x => x != null) // backward compat, don't store null values in the cache - .ToList(); + .ToArray(); // evaluate while locked } + 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 } public virtual object GetCacheItem(string cacheKey) { cacheKey = GetCacheKey(cacheKey); - using (new ReadLock(Locker)) + Lazy result; + using (ReadLock) { - var result = GetEntry(cacheKey) as Lazy; // null if key not found - return result == null ? null : GetSafeLazyValue(result); // return exceptions as null + result = GetEntry(cacheKey) as Lazy; // null if key not found } + return result == null ? null : GetSafeLazyValue(result); // return exceptions as null } public abstract object GetCacheItem(string cacheKey, Func getCacheItem); diff --git a/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs index 9857633b8c..ea5969fb8c 100644 --- a/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs @@ -11,37 +11,91 @@ namespace Umbraco.Core.Cache /// internal class HttpRequestCacheProvider : DictionaryCacheProviderBase { - private readonly Func _context; + // context provider + // the idea is that there is only one, application-wide HttpRequestCacheProvider instance, + // that is initialized with a method that returns the "current" context. + // NOTE + // but then it is initialized with () => new HttpContextWrapper(HttpContent.Current) + // which is higly inefficient because it creates a new wrapper each time we refer to _context() + // so replace it with _context1 and _context2 below + a way to get context.Items. + //private readonly Func _context; - public HttpRequestCacheProvider(HttpContext context) + // NOTE + // and then in almost 100% cases _context2 will be () => HttpContext.Current + // so why not bring that logic in here and fallback on to HttpContext.Current when + // _context1 is null? + //private readonly HttpContextBase _context1; + //private readonly Func _context2; + private readonly HttpContextBase _context; + + private IDictionary ContextItems { - // create wrapper only once! - var wrapper = new HttpContextWrapper(context); - _context = () => wrapper; + //get { return _context1 != null ? _context1.Items : _context2().Items; } + get { return _context != null ? _context.Items : HttpContext.Current.Items; } } - public HttpRequestCacheProvider(Func context) + // for unit tests + public HttpRequestCacheProvider(HttpContextBase context) { _context = context; } + // main constructor + // will use HttpContext.Current + public HttpRequestCacheProvider(/*Func context*/) + { + //_context2 = context; + } + protected override IEnumerable GetDictionaryEntries() { const string prefix = CacheItemPrefix + "-"; - return _context().Items.Cast() + return ContextItems.Cast() .Where(x => x.Key is string && ((string)x.Key).StartsWith(prefix)); } protected override void RemoveEntry(string key) { - _context().Items.Remove(key); + ContextItems.Remove(key); } protected override object GetEntry(string key) { - return _context().Items[key]; + return ContextItems[key]; } + #region Lock + + protected override IDisposable ReadLock + { + // there's no difference between ReadLock and WriteLock here + get { return WriteLock; } + } + + 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. + + get + { + // FIXME + // we still need something to lock though + // some locker in the context items? + + object locker; + lock (this) + { + locker = ContextItems["locker"] ?? (ContextItems["locker"] = new object()); + } + return new MonitorLock(locker); + } + } + + #endregion + #region Get public override object GetCacheItem(string cacheKey, Func getCacheItem) @@ -50,15 +104,18 @@ namespace Umbraco.Core.Cache Lazy result; - using (var lck = new UpgradeableReadLock(Locker)) + using (WriteLock) { - result = _context().Items[cacheKey] as Lazy; // null if key not found - if (result == null || (result.IsValueCreated && GetSafeLazyValue(result) == null)) // get exceptions as null - { - lck.UpgradeToWriteLock(); + result = ContextItems[cacheKey] as Lazy; // null if key not found + // cannot create value within the lock, so if result.IsValueCreated is false, just + // do nothing here - means that if creation throws, a race condition could cause + // more than one thread to reach the return statement below and throw - accepted. + + if (result == null || GetSafeLazyValue(result, true) == null) // get non-created as NonCreatedValue & exceptions as null + { result = new Lazy(getCacheItem); - _context().Items[cacheKey] = result; + ContextItems[cacheKey] = result; } } diff --git a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs index 79c8116b17..c5870f26e8 100644 --- a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs @@ -2,6 +2,7 @@ using System.Collections; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Web.Caching; using CacheItemPriority = System.Web.Caching.CacheItemPriority; @@ -12,6 +13,11 @@ namespace Umbraco.Core.Cache /// internal class HttpRuntimeCacheProvider : DictionaryCacheProviderBase, IRuntimeCacheProvider { + // locker object that supports upgradeable read locking + // does not need to support recursion if we implement the cache correctly and ensure + // that methods cannot be reentrant, ie we do NOT create values while holding a lock. + private readonly ReaderWriterLockSlim _locker = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion); + private readonly System.Web.Caching.Cache _cache; public HttpRuntimeCacheProvider(System.Web.Caching.Cache cache) @@ -36,6 +42,20 @@ namespace Umbraco.Core.Cache return _cache.Get(key); } + #region Lock + + protected override IDisposable ReadLock + { + get { return new ReadLock(_locker); } + } + + protected override IDisposable WriteLock + { + get { return new WriteLock(_locker); } + } + + #endregion + #region Get /// @@ -72,26 +92,48 @@ namespace Umbraco.Core.Cache // 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 the Lazy execution may produce a null value. - // Must make sure (for backward compatibility) that we pretend they are not in the cache. - // So if we find an entry in the cache that already has its value created and is null, - // pretend it was not there. If value is not already created, wait... and return null, that's - // what prior code did. + // NOTE + // The Lazy value creation may produce a null value. + // Must make sure (for backward compatibility) that we pretend they are not in the cache. + // So if we find an entry in the cache that already has its value created and is null, + // pretend it was not there. If value is not already created, wait... and return null, that's + // what prior code did. + + // NOTE + // The Lazy value creation may throw. // So... the null value _will_ be in the cache but never returned Lazy result; - using (var lck = new UpgradeableReadLock(Locker)) + // Fast! + // Only one thread can enter an UpgradeableReadLock at a time, but it does not prevent other + // threads to enter a ReadLock in the meantime -- only upgrading to WriteLock will prevent all + // 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)) { result = _cache.Get(cacheKey) as Lazy; // null if key not found - if (result == null || (result.IsValueCreated && GetSafeLazyValue(result) == null)) // get exceptions as null - { - lck.UpgradeToWriteLock(); + } + var value = result == null ? null : GetSafeLazyValue(result); + if (value != null) return value; + using (var lck = new UpgradeableReadLock(_locker)) + { + result = _cache.Get(cacheKey) as Lazy; // null if key not found + + // cannot create value within the lock, so if result.IsValueCreated is false, just + // do nothing here - means that if creation throws, a race condition could cause + // more than one thread to reach the return statement below and throw - accepted. + + if (result == null || GetSafeLazyValue(result, true) == null) // get non-created as NonCreatedValue & exceptions as null + { 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); + + lck.UpgradeToWriteLock(); _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); } } @@ -140,7 +182,10 @@ 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); - _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); + using (new WriteLock(_locker)) + { + _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); + } } 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/CacheHelper.cs b/src/Umbraco.Core/CacheHelper.cs index 539ac796b7..6dd123e15f 100644 --- a/src/Umbraco.Core/CacheHelper.cs +++ b/src/Umbraco.Core/CacheHelper.cs @@ -44,7 +44,7 @@ namespace Umbraco.Core : this( new HttpRuntimeCacheProvider(HttpRuntime.Cache), new StaticCacheProvider(), - new HttpRequestCacheProvider(() => new HttpContextWrapper(HttpContext.Current))) + new HttpRequestCacheProvider()) { } @@ -56,7 +56,7 @@ namespace Umbraco.Core : this( new HttpRuntimeCacheProvider(cache), new StaticCacheProvider(), - new HttpRequestCacheProvider(() => new HttpContextWrapper(HttpContext.Current))) + new HttpRequestCacheProvider()) { } diff --git a/src/Umbraco.Core/MonitorLock.cs b/src/Umbraco.Core/MonitorLock.cs new file mode 100644 index 0000000000..9d17c86be8 --- /dev/null +++ b/src/Umbraco.Core/MonitorLock.cs @@ -0,0 +1,32 @@ +using System; + +namespace Umbraco.Core +{ + /// + /// Provides an equivalent to the c# lock statement, to be used in a using block. + /// + /// Ie replace lock (o) {...} by using (new MonitorLock(o)) { ... } + public class MonitorLock : IDisposable + { + private readonly object _locker; + private readonly bool _entered; + + /// + /// Initializes a new instance of the class with an object to lock. + /// + /// The object to lock. + /// Should always be used within a using block. + public MonitorLock(object locker) + { + _locker = locker; + _entered = false; + System.Threading.Monitor.Enter(_locker, ref _entered); + } + + void IDisposable.Dispose() + { + if (_entered) + System.Threading.Monitor.Exit(_locker); + } + } +} diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 2c5ac85ba6..fed4b266be 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -215,6 +215,7 @@ + diff --git a/src/Umbraco.Tests/Cache/HttpRequestCacheProviderTests.cs b/src/Umbraco.Tests/Cache/HttpRequestCacheProviderTests.cs index dcbddbb44e..8fbcf1d205 100644 --- a/src/Umbraco.Tests/Cache/HttpRequestCacheProviderTests.cs +++ b/src/Umbraco.Tests/Cache/HttpRequestCacheProviderTests.cs @@ -14,7 +14,7 @@ namespace Umbraco.Tests.Cache { base.Setup(); _ctx = new FakeHttpContextFactory("http://localhost/test"); - _provider = new HttpRequestCacheProvider(() => _ctx.HttpContext); + _provider = new HttpRequestCacheProvider(_ctx.HttpContext); } internal override ICacheProvider Provider