U4-6597 - fix issue with caches that would cache exceptions

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<object> lazy, bool onlyIfValueIsCreated = false)
+        protected internal static Lazy<object> GetSafeLazy(Func<object> 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<object>(() =>
             {
-                // if onlyIfValueIsCreated, do not trigger value creation
+                try
-                // must return something, though, to differenciate from null values
+                {
-                if (onlyIfValueIsCreated && lazy.IsValueCreated == false) return ValueNotCreated;
+                    return getCacheItem();
-                return lazy.Value;
+                }
-            }
+                catch (Exception e)
-            catch
+                {
+                    return new ExceptionHolder(e);
+                }
+            });
+        }
+
+        protected internal static object GetSafeLazyValue(Lazy<object> 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<T> 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

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<object>(getCacheItem);
+                    result = GetSafeLazy(getCacheItem);
                     ContextItems[cacheKey] = result;
                 }
             }
 
-            // this may throw if getCacheItem throws, but this is the only place where
+            // using GetSafeLazy and GetSafeLazyValue ensures that we don't cache
-            // it would throw as everywhere else we use GetLazySaveValue() to hide exceptions
+            // exceptions (but try again and again) and silently eat them - however at
-            // and pretend exceptions were never inserted into cache to begin with.
+            // some point we have to report them - so need to re-throw here
-            return result.Value;
+
+            // 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

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<object>(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
+            // using GetSafeLazy and GetSafeLazyValue ensures that we don't cache
-            // it would throw as everywhere else we use GetLazySaveValue() to hide exceptions
+            // exceptions (but try again and again) and silently eat them - however at
-            // and pretend exceptions were never inserted into cache to begin with.
+            // some point we have to report them - so need to re-throw here
-            return result.Value;
+
+            // 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<object> 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<object> but we can evaluate it right now
             // and make sure we don't store a null value.
 
-            var result = new Lazy<object>(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)
 

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<object> 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<object> 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<object>)x.Value, true);
+                        var value = DictionaryCacheProviderBase.GetSafeLazyValue((Lazy<object>)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<object> 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<object>)x.Value, true);
+                        var value = DictionaryCacheProviderBase.GetSafeLazyValue((Lazy<object>)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<object> 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<object>)x.Value, true);
+                        var value = DictionaryCacheProviderBase.GetSafeLazyValue((Lazy<object>)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<object>)x.Value)) // return exceptions as null
+                .Select(x => DictionaryCacheProviderBase.GetSafeLazyValue((Lazy<object>)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<object>)x.Value)) // return exceptions as null
+                .Select(x => DictionaryCacheProviderBase.GetSafeLazyValue((Lazy<object>)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<object>; // 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<object> getCacheItem)
@@ -205,9 +187,9 @@ namespace Umbraco.Core.Cache
             using (var lck = new UpgradeableReadLock(_locker))
             {
                 result = MemoryCache.Get(cacheKey) as Lazy<object>;
-                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<object>(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<object> but we can evaluate it right now
             // and make sure we don't store a null value.
 
-            var result = new Lazy<object>(getCacheItem);
+            var result = DictionaryCacheProviderBase.GetSafeLazy(getCacheItem);
             var value = result.Value; // force evaluation now
             if (value == null) return; // do not store null values (backward compat)
 

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()
         {

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<Exception>(() => { value = (string)_provider.GetCacheItem("key", () => GetValue(1)); });
+            Assert.Throws<Exception>(() => { 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;
+        }
     }
 }

src/Umbraco.Web.UI/config/umbracoSettings.config

@@ -118,33 +118,10 @@
     <useDomainPrefixes>false</useDomainPrefixes>
     <!-- this will add a trailing slash (/) to urls when in directory url mode -->
     <addTrailingSlash>true</addTrailingSlash>
-    <urlReplacing removeDoubleDashes="true">
+    <urlReplacing removeDoubleDashes="true" toAscii="true">
       <char org=" ">-</char>
-      <char org="&quot;"></char>
+      <char org="ö">oxxx</char>
-      <char org="'"></char>
+      <!--<char org="ü">ue</char>-->
-      <char org="%"></char>
-      <char org="."></char>
-      <char org=";"></char>
-      <char org="/"></char>
-      <char org="\"></char>
-      <char org=":"></char>
-      <char org="#"></char>
-      <char org="+">plus</char>
-      <char org="*">star</char>
-      <char org="&amp;"></char>
-      <char org="?"></char>
-      <char org="æ">ae</char>
-      <char org="ø">oe</char>
-      <char org="å">aa</char>
-      <char org="ä">ae</char>
-      <char org="ö">oe</char>
-      <char org="ü">ue</char>
-      <char org="ß">ss</char>
-      <char org="Ä">ae</char>
-      <char org="Ö">oe</char>
-      <char org="|">-</char>
-      <char org="&lt;"></char>
-      <char org="&gt;"></char>
     </urlReplacing>
   </requestHandler>