Improving monitors usage for output cache

src/Orchard.Web/Modules/Orchard.OutputCache/Filters/OutputCacheFilter.cs

@@ -1,5 +1,4 @@
 using System;
-using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Collections.Specialized;
 using System.Globalization;
@@ -24,11 +23,10 @@ using Orchard.UI.Admin;
 using Orchard.Utility.Extensions;
 
 namespace Orchard.OutputCache.Filters {
-    public class OutputCacheFilter : FilterProvider, IActionFilter, IResultFilter {
+    public class OutputCacheFilter : FilterProvider, IActionFilter, IResultFilter, IDisposable {
 
         private static string _refreshKey = "__r";
         private static long _epoch = new DateTime(2014, DateTimeKind.Utc).Ticks;
-        private static readonly ConcurrentDictionary<string, object> _cacheKeyLocks = new ConcurrentDictionary<string, object>();
 
         // Dependencies.
         private readonly ICacheManager _cacheManager;
@@ -41,6 +39,8 @@ namespace Orchard.OutputCache.Filters {
         private readonly ICacheService _cacheService;
         private readonly ISignals _signals;
         private readonly ShellSettings _shellSettings;
+        private bool _isDisposed = false;
+
         public ILogger Logger { get; set; }
 
         public OutputCacheFilter(
@@ -96,15 +96,11 @@ namespace Orchard.OutputCache.Filters {
                 return;
 
             // Computing the cache key after we know that the request is cacheable means that we are only performing this calculation on requests that require it
-            _cacheKey = ComputeCacheKey(filterContext, GetCacheKeyParameters(filterContext));
+            _cacheKey = String.Intern(ComputeCacheKey(filterContext, GetCacheKeyParameters(filterContext)));
             _invariantCacheKey = ComputeCacheKey(filterContext, null);
 
             Logger.Debug("Cache key '{0}' was created.", _cacheKey);
 
-            // The cache key lock for a given cache key is used to synchronize requests to
-            // ensure only a single request is regenerating the item.
-            var cacheKeyLock = _cacheKeyLocks.GetOrAdd(_cacheKey, x => new object());
-
             try {
 
                 // Is there a cached item, and are we allowed to serve it?
@@ -118,7 +114,7 @@ namespace Orchard.OutputCache.Filters {
                     if (cacheItem.IsInGracePeriod(_now)) {
 
                         // Render the content unless another request is already doing so.
-                        if (Monitor.TryEnter(cacheKeyLock)) {
+                        if (Monitor.TryEnter(_cacheKey)) {
                             Logger.Debug("Item '{0}' is in grace period and not currently being rendered; rendering item...", _cacheKey);
                             BeginRenderItem(filterContext);
                             return;
@@ -135,7 +131,7 @@ namespace Orchard.OutputCache.Filters {
                 // No cached item found, or client doesn't want it; acquire the cache key
                 // lock to render the item.
                 Logger.Debug("Item '{0}' was not found in cache or client refuses it. Acquiring cache key lock...", _cacheKey);
-                if (Monitor.TryEnter(cacheKeyLock, TimeSpan.FromSeconds(20))) {
+                if (Monitor.TryEnter(_cacheKey)) {
                     Logger.Debug("Cache key lock for item '{0}' was acquired.", _cacheKey);
 
                     // Item might now have been rendered and cached by another request; if so serve it from cache.
@@ -143,7 +139,7 @@ namespace Orchard.OutputCache.Filters {
                         cacheItem = GetCacheItem(_cacheKey);
                         if (cacheItem != null) {
                             Logger.Debug("Item '{0}' was now found; releasing cache key lock and serving from cache.", _cacheKey);
-                            Monitor.Exit(cacheKeyLock);
+                            Monitor.Exit(_cacheKey);
                             ServeCachedItem(filterContext, cacheItem);
                             return;
                         }
@@ -159,8 +155,7 @@ namespace Orchard.OutputCache.Filters {
             catch {
                 // Remember to release the cache key lock in the event of an exception!
                 Logger.Debug("Exception occurred for item '{0}'; releasing any acquired lock.", _cacheKey);
-                if (Monitor.IsEntered(cacheKeyLock))
+                ReleaseCacheKeyLock();
-                    Monitor.Exit(cacheKeyLock);
                 throw;
             }
         }
@@ -177,11 +172,11 @@ namespace Orchard.OutputCache.Filters {
             var captureHandlerIsAttached = false;
 
             try {
-
+                
                 // This filter is not reentrant (multiple executions within the same request are
                 // not supported) so child actions are ignored completely.
                 if (filterContext.IsChildAction || !_isCachingRequest)
-                    return;
+                return;
 
                 Logger.Debug("Item '{0}' was rendered.", _cacheKey);
 
@@ -358,7 +353,7 @@ namespace Orchard.OutputCache.Filters {
 
         protected virtual IDictionary<string, object> GetCacheKeyParameters(ActionExecutingContext filterContext) {
             var result = new Dictionary<string, object>();
-            
+
             // Vary by action parameters.
             foreach (var p in filterContext.ActionParameters)
                 result.Add("PARAM:" + p.Key, p.Value);
@@ -381,7 +376,7 @@ namespace Orchard.OutputCache.Filters {
                     result["HEADER:" + varyByRequestHeader] = requestHeaders[varyByRequestHeader];
             }
 
-            
+
             // Vary by request culture if configured.
             if (CacheSettings.VaryByCulture) {
                 result["culture"] = _workContext.CurrentCulture.ToLowerInvariant();
@@ -472,7 +467,7 @@ namespace Orchard.OutputCache.Filters {
             var response = filterContext.HttpContext.Response;
 
             // Fix for missing charset in response headers
-            response.Charset = response.Charset; 
+            response.Charset = response.Charset;
 
             // Adds some caching information to the output if requested.
             if (CacheSettings.DebugMode) {
@@ -538,12 +533,10 @@ namespace Orchard.OutputCache.Filters {
         }
 
         private void ReleaseCacheKeyLock() {
-            if (_cacheKey != null) {
+            if (_cacheKey != null && Monitor.IsEntered(_cacheKey)) {
-                object cacheKeyLock;
+                Logger.Debug("Releasing cache key lock for item '{0}'.", _cacheKey);
-                if (_cacheKeyLocks.TryGetValue(_cacheKey, out cacheKeyLock) && Monitor.IsEntered(cacheKeyLock)) {
+                Monitor.Exit(_cacheKey);
-                    Logger.Debug("Releasing cache key lock for item '{0}'.", _cacheKey);
+                _cacheKey = null;
-                    Monitor.Exit(cacheKeyLock);
-                }
             }
         }
 
@@ -597,12 +590,37 @@ namespace Orchard.OutputCache.Filters {
                 var cacheItem = _cacheStorageProvider.GetCacheItem(key);
                 return cacheItem;
             }
-            catch(Exception e) {
+            catch (Exception e) {
                 Logger.Error(e, "An unexpected error occured while reading a cache entry");
             }
 
             return null;
         }
+
+        public void Dispose() {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
+
+        protected virtual void Dispose(bool disposing) {
+            if (!_isDisposed) {
+                if (disposing) {
+                    // Free other state (managed objects).
+                }
+
+                if (_cacheKey != null && Monitor.IsEntered(_cacheKey)) {
+                    Monitor.Exit(_cacheKey);
+                }
+
+                _isDisposed = true;
+            }
+        }
+
+        ~OutputCacheFilter() {
+            // Ensure locks are released even after an unexpected exception
+            Dispose(false);
+        }
+        
     }
 
     public class ViewDataContainer : IViewDataContainer {