From 6fef85aafde5eb6614c9831034ac1a9cc11eabf6 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 26 Mar 2020 22:03:26 +1100 Subject: [PATCH 1/2] fixes Memory Leak in GetCacheItem cache dependency #7773 --- src/Umbraco.Core/Cache/WebCachingAppCache.cs | 37 +++++++++++--------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/Umbraco.Core/Cache/WebCachingAppCache.cs b/src/Umbraco.Core/Cache/WebCachingAppCache.cs index 044aab4c42..87f0e75845 100644 --- a/src/Umbraco.Core/Cache/WebCachingAppCache.cs +++ b/src/Umbraco.Core/Cache/WebCachingAppCache.cs @@ -37,23 +37,28 @@ namespace Umbraco.Core.Cache /// public object Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - CacheDependency dependency = null; - if (dependentFiles != null && dependentFiles.Any()) - { - dependency = new CacheDependency(dependentFiles); - } - return Get(key, factory, timeout, isSliding, priority, removedCallback, dependency); + return Get(key, factory, timeout, isSliding, priority, removedCallback, + // NOTE: We don't want to allocate an object if it isn't going to be used so we create the CacheDependency + // in a callback if it's needed ... but more importantly and we didn't anticipate this, just constructing + // a CacheDependency object is expensive and allocates a bunch of stuff, just check the code out for it: + // https://referencesource.microsoft.com/#system.web/Cache/CacheDependency.cs,304 + // Init is called as part of the ctor!! yikes. + // This change fixes https://github.com/umbraco/Umbraco-CMS/issues/7773 + () => dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null); } /// public void Insert(string key, Func factory, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - CacheDependency dependency = null; - if (dependentFiles != null && dependentFiles.Any()) - { - dependency = new CacheDependency(dependentFiles); - } - Insert(key, factory, timeout, isSliding, priority, removedCallback, dependency); + + Insert(key, factory, timeout, isSliding, priority, removedCallback, + // NOTE: We don't want to allocate an object if it isn't going to be used so we create the CacheDependency + // in a callback if it's needed ... but more importantly and we didn't anticipate this, just constructing + // a CacheDependency object is expensive and allocates a bunch of stuff, just check the code out for it: + // https://referencesource.microsoft.com/#system.web/Cache/CacheDependency.cs,304 + // Init is called as part of the ctor!! yikes. + // This change fixes https://github.com/umbraco/Umbraco-CMS/issues/7773 + () => dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null); } #region Dictionary @@ -103,7 +108,7 @@ namespace Umbraco.Core.Cache #endregion - private object Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, CacheDependency dependency = null) + private object Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, Func dependency = null) { key = GetCacheKey(key); @@ -164,7 +169,7 @@ namespace Umbraco.Core.Cache lck.UpgradeToWriteLock(); //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! - _cache.Insert(key, result, dependency, absolute, sliding, priority, removedCallback); + _cache.Insert(key, result, dependency(), absolute, sliding, priority, removedCallback); } } @@ -180,7 +185,7 @@ namespace Umbraco.Core.Cache return value; } - private void Insert(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, CacheDependency dependency = null) + private void Insert(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, Func dependency = null) { // 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. @@ -198,7 +203,7 @@ namespace Umbraco.Core.Cache { _locker.EnterWriteLock(); //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! - _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); + _cache.Insert(cacheKey, result, dependency(), absolute, sliding, priority, removedCallback); } finally { From a855a90a8a5b06eff0a91008e64d446379fb94ca Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 26 Mar 2020 22:09:49 +1100 Subject: [PATCH 2/2] cleaner fix --- src/Umbraco.Core/Cache/WebCachingAppCache.cs | 35 ++++++++------------ 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/Umbraco.Core/Cache/WebCachingAppCache.cs b/src/Umbraco.Core/Cache/WebCachingAppCache.cs index 87f0e75845..bec596b129 100644 --- a/src/Umbraco.Core/Cache/WebCachingAppCache.cs +++ b/src/Umbraco.Core/Cache/WebCachingAppCache.cs @@ -37,28 +37,13 @@ namespace Umbraco.Core.Cache /// public object Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - return Get(key, factory, timeout, isSliding, priority, removedCallback, - // NOTE: We don't want to allocate an object if it isn't going to be used so we create the CacheDependency - // in a callback if it's needed ... but more importantly and we didn't anticipate this, just constructing - // a CacheDependency object is expensive and allocates a bunch of stuff, just check the code out for it: - // https://referencesource.microsoft.com/#system.web/Cache/CacheDependency.cs,304 - // Init is called as part of the ctor!! yikes. - // This change fixes https://github.com/umbraco/Umbraco-CMS/issues/7773 - () => dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null); + return GetInternal(key, factory, timeout, isSliding, priority, removedCallback, dependentFiles); } /// public void Insert(string key, Func factory, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - - Insert(key, factory, timeout, isSliding, priority, removedCallback, - // NOTE: We don't want to allocate an object if it isn't going to be used so we create the CacheDependency - // in a callback if it's needed ... but more importantly and we didn't anticipate this, just constructing - // a CacheDependency object is expensive and allocates a bunch of stuff, just check the code out for it: - // https://referencesource.microsoft.com/#system.web/Cache/CacheDependency.cs,304 - // Init is called as part of the ctor!! yikes. - // This change fixes https://github.com/umbraco/Umbraco-CMS/issues/7773 - () => dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null); + InsertInternal(key, factory, timeout, isSliding, priority, removedCallback, dependentFiles); } #region Dictionary @@ -108,7 +93,7 @@ namespace Umbraco.Core.Cache #endregion - private object Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, Func dependency = null) + private object GetInternal(string key, Func factory, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { key = GetCacheKey(key); @@ -168,8 +153,12 @@ namespace Umbraco.Core.Cache var sliding = isSliding == false ? System.Web.Caching.Cache.NoSlidingExpiration : (timeout ?? System.Web.Caching.Cache.NoSlidingExpiration); lck.UpgradeToWriteLock(); + + // create a cache dependency if one is needed. + var dependency = dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null; + //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! - _cache.Insert(key, result, dependency(), absolute, sliding, priority, removedCallback); + _cache.Insert(key, result, dependency, absolute, sliding, priority, removedCallback); } } @@ -185,7 +174,7 @@ namespace Umbraco.Core.Cache return value; } - private void Insert(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, Func dependency = null) + private void InsertInternal(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { // 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. @@ -202,8 +191,12 @@ namespace Umbraco.Core.Cache try { _locker.EnterWriteLock(); + + // create a cache dependency if one is needed. + var dependency = dependentFiles != null && dependentFiles.Length > 0 ? new CacheDependency(dependentFiles) : null; + //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! - _cache.Insert(cacheKey, result, dependency(), absolute, sliding, priority, removedCallback); + _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); } finally {