diff --git a/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs b/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs index a8307044a1..64eb8a3d35 100644 --- a/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs +++ b/src/Umbraco.Core/Cache/DictionaryCacheProviderBase.cs @@ -12,7 +12,7 @@ namespace Umbraco.Core.Cache protected const string CacheItemPrefix = "umbrtmche"; // an object that represent a value that has not been created yet - protected readonly object ValueNotCreated = new object(); + protected internal static readonly object ValueNotCreated = new object(); // manupulate the underlying cache entries // these *must* be called from within the appropriate locks @@ -30,19 +30,49 @@ namespace Umbraco.Core.Cache return string.Format("{0}-{1}", CacheItemPrefix, key); } - protected object GetSafeLazyValue(Lazy lazy, bool onlyIfValueIsCreated = false) + protected internal static Lazy GetSafeLazy(Func getCacheItem) { - try + // try to generate the value and if it fails, + // wrap in an ExceptionHolder - would be much simpler + // to just use lazy.IsValueFaulted alas that field is + // internal + return new Lazy(() => { - // 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 + try + { + return getCacheItem(); + } + catch (Exception e) + { + return new ExceptionHolder(e); + } + }); + } + + protected internal static object GetSafeLazyValue(Lazy lazy, bool onlyIfValueIsCreated = false) + { + // if onlyIfValueIsCreated, do not trigger value creation + // must return something, though, to differenciate from null values + if (onlyIfValueIsCreated && lazy.IsValueCreated == false) return ValueNotCreated; + + // if execution has thrown then lazy.IsValueCreated is false + // and lazy.IsValueFaulted is true (but internal) so we use our + // own exception holder (see Lazy source code) to return null + if (lazy.Value is ExceptionHolder) return null; + + // we have a value and execution has not thrown so returning + // here does not throw + return lazy.Value; + } + + internal class ExceptionHolder + { + public ExceptionHolder(Exception e) { - return null; + Exception = e; } + + public Exception Exception { get; private set; } } #region Clear diff --git a/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs index 0a95ff6fd2..a4a6938fc0 100644 --- a/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRequestCacheProvider.cs @@ -105,15 +105,22 @@ namespace Umbraco.Core.Cache if (result == null || GetSafeLazyValue(result, true) == null) // get non-created as NonCreatedValue & exceptions as null { - result = new Lazy(getCacheItem); + result = GetSafeLazy(getCacheItem); ContextItems[cacheKey] = result; } } - // this may throw if getCacheItem throws, but this is the only place where - // it would throw as everywhere else we use GetLazySaveValue() to hide exceptions - // and pretend exceptions were never inserted into cache to begin with. - return result.Value; + // using GetSafeLazy and GetSafeLazyValue ensures that we don't cache + // exceptions (but try again and again) and silently eat them - however at + // some point we have to report them - so need to re-throw here + + // this does not throw anymore + //return result.Value; + + var value = result.Value; // will not throw (safe lazy) + var eh = value as ExceptionHolder; + if (eh != null) throw eh.Exception; // throw once! + return value; } #endregion diff --git a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs index c5870f26e8..748912b9f1 100644 --- a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs @@ -129,7 +129,7 @@ namespace Umbraco.Core.Cache if (result == null || GetSafeLazyValue(result, true) == null) // get non-created as NonCreatedValue & exceptions as null { - result = new Lazy(getCacheItem); + result = GetSafeLazy(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); @@ -138,10 +138,17 @@ namespace Umbraco.Core.Cache } } - // this may throw if getCacheItem throws, but this is the only place where - // it would throw as everywhere else we use GetLazySaveValue() to hide exceptions - // and pretend exceptions were never inserted into cache to begin with. - return result.Value; + // using GetSafeLazy and GetSafeLazyValue ensures that we don't cache + // exceptions (but try again and again) and silently eat them - however at + // some point we have to report them - so need to re-throw here + + // this does not throw anymore + //return result.Value; + + value = result.Value; // will not throw (safe lazy) + var eh = value as ExceptionHolder; + if (eh != null) throw eh.Exception; // throw once! + return value; } public object GetCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) @@ -173,7 +180,7 @@ namespace Umbraco.Core.Cache // 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. - var result = new Lazy(getCacheItem); + var result = GetSafeLazy(getCacheItem); var value = result.Value; // force evaluation now - this may throw if cacheItem throws, and then nothing goes into cache if (value == null) return; // do not store null values (backward compat) diff --git a/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs index edf1ba5aa6..d4f2c33ed1 100644 --- a/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs @@ -19,29 +19,11 @@ namespace Umbraco.Core.Cache private readonly ReaderWriterLockSlim _locker = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion); internal ObjectCache MemoryCache; - // an object that represent a value that has not been created yet - protected readonly object ValueNotCreated = new object(); - public ObjectCacheRuntimeCacheProvider() { MemoryCache = new MemoryCache("in-memory"); } - 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 - { - return null; - } - } - #region Clear public virtual void ClearAllCache() @@ -72,7 +54,7 @@ namespace Umbraco.Core.Cache // x.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 = DictionaryCacheProviderBase.GetSafeLazyValue((Lazy)x.Value, true); return value == null || value.GetType().ToString().InvariantEquals(typeName); }) .Select(x => x.Key) @@ -92,7 +74,7 @@ namespace Umbraco.Core.Cache // x.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 = DictionaryCacheProviderBase.GetSafeLazyValue((Lazy)x.Value, true); return value == null || value.GetType() == typeOfT; }) .Select(x => x.Key) @@ -112,7 +94,7 @@ namespace Umbraco.Core.Cache // x.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 = DictionaryCacheProviderBase.GetSafeLazyValue((Lazy)x.Value, true); if (value == null) return true; return value.GetType() == typeOfT && predicate(x.Key, (T) value); @@ -161,7 +143,7 @@ namespace Umbraco.Core.Cache .ToArray(); // evaluate while locked } return entries - .Select(x => GetSafeLazyValue((Lazy)x.Value)) // return exceptions as null + .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 .ToList(); } @@ -176,7 +158,7 @@ namespace Umbraco.Core.Cache .ToArray(); // evaluate while locked } return entries - .Select(x => GetSafeLazyValue((Lazy)x.Value)) // return exceptions as null + .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 .ToList(); } @@ -188,7 +170,7 @@ namespace Umbraco.Core.Cache { result = MemoryCache.Get(cacheKey) as Lazy; // null if key not found } - return result == null ? null : GetSafeLazyValue(result); // return exceptions as null + return result == null ? null : DictionaryCacheProviderBase.GetSafeLazyValue(result); // return exceptions as null } public object GetCacheItem(string cacheKey, Func getCacheItem) @@ -205,9 +187,9 @@ namespace Umbraco.Core.Cache using (var lck = new UpgradeableReadLock(_locker)) { result = MemoryCache.Get(cacheKey) as Lazy; - if (result == null || GetSafeLazyValue(result, true) == null) // get non-created as NonCreatedValue & exceptions as null + if (result == null || DictionaryCacheProviderBase.GetSafeLazyValue(result, true) == null) // get non-created as NonCreatedValue & exceptions as null { - result = new Lazy(getCacheItem); + result = DictionaryCacheProviderBase.GetSafeLazy(getCacheItem); var policy = GetPolicy(timeout, isSliding, removedCallback, dependentFiles); lck.UpgradeToWriteLock(); @@ -215,7 +197,12 @@ namespace Umbraco.Core.Cache } } - return result.Value; + //return result.Value; + + var value = result.Value; // will not throw (safe lazy) + var eh = value as DictionaryCacheProviderBase.ExceptionHolder; + if (eh != null) throw eh.Exception; // throw once! + return value; } #endregion @@ -227,7 +214,7 @@ namespace Umbraco.Core.Cache // 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. - var result = new Lazy(getCacheItem); + var result = DictionaryCacheProviderBase.GetSafeLazy(getCacheItem); var value = result.Value; // force evaluation now if (value == null) return; // do not store null values (backward compat) diff --git a/src/Umbraco.Tests/Cache/CacheProviderTests.cs b/src/Umbraco.Tests/Cache/CacheProviderTests.cs index b0a8aec68c..8eab3b4bde 100644 --- a/src/Umbraco.Tests/Cache/CacheProviderTests.cs +++ b/src/Umbraco.Tests/Cache/CacheProviderTests.cs @@ -1,4 +1,5 @@ -using System.Linq; +using System; +using System.Linq; using System.Web.UI; using NUnit.Framework; using Umbraco.Core.Cache; @@ -23,6 +24,59 @@ namespace Umbraco.Tests.Cache Provider.ClearAllCache(); } + [Test] + public void Does_Not_Cache_Exceptions() + { + var counter = 0; + + object result; + try + { + result = Provider.GetCacheItem("Blah", () => + { + counter++; + throw new Exception("Do not cache this"); + }); + } + catch (Exception){} + + try + { + result = Provider.GetCacheItem("Blah", () => + { + counter++; + throw new Exception("Do not cache this"); + }); + } + catch (Exception){} + + Assert.Greater(counter, 1); + + } + + [Test] + public void Ensures_Delegate_Result_Is_Cached_Once() + { + var counter = 0; + + object result; + + result = Provider.GetCacheItem("Blah", () => + { + counter++; + return ""; + }); + + result = Provider.GetCacheItem("Blah", () => + { + counter++; + return ""; + }); + + Assert.AreEqual(counter, 1); + + } + [Test] public void Can_Get_By_Search() { diff --git a/src/Umbraco.Tests/Cache/HttpRuntimeCacheProviderTests.cs b/src/Umbraco.Tests/Cache/HttpRuntimeCacheProviderTests.cs index 1fc8ab1327..87e6d4366e 100644 --- a/src/Umbraco.Tests/Cache/HttpRuntimeCacheProviderTests.cs +++ b/src/Umbraco.Tests/Cache/HttpRuntimeCacheProviderTests.cs @@ -1,4 +1,5 @@ -using System.Web; +using System; +using System.Web; using NUnit.Framework; using Umbraco.Core.Cache; @@ -29,5 +30,29 @@ namespace Umbraco.Tests.Cache { get { return _provider; } } + + [Test] + public void DoesNotCacheExceptions() + { + string value; + Assert.Throws(() => { value = (string)_provider.GetCacheItem("key", () => GetValue(1)); }); + Assert.Throws(() => { value = (string)_provider.GetCacheItem("key", () => GetValue(2)); }); + + // does not throw + value = (string)_provider.GetCacheItem("key", () => GetValue(3)); + Assert.AreEqual("succ3", value); + + // cache + value = (string)_provider.GetCacheItem("key", () => GetValue(4)); + Assert.AreEqual("succ3", value); + } + + private static string GetValue(int i) + { + Console.WriteLine("get" + i); + if (i < 3) + throw new Exception("fail"); + return "succ" + i; + } } } \ No newline at end of file diff --git a/src/Umbraco.Web.UI/config/umbracoSettings.config b/src/Umbraco.Web.UI/config/umbracoSettings.config index 616b5ba424..255dce6542 100644 --- a/src/Umbraco.Web.UI/config/umbracoSettings.config +++ b/src/Umbraco.Web.UI/config/umbracoSettings.config @@ -118,33 +118,10 @@ false true - + - - - - - - - - - - - plus - star - - - ae - oe - aa - ae - oe - ue - ss - ae - oe - - - - + oxxx +