Memory Leak in Microsoft.Extensions.Caching.Memory when handling exce…

…ptions

When an exception is thrown inside MemoryCache.GetOrCreate, we are leaking CacheEntry objects. This is because they are not being Disposed properly, and the async local CacheEntryStack is growing indefinitely.

The fix is to ensure the CacheEntry objects are disposed correctly. In order to do this, I set a flag to indicate whether the CacheEntry.Value has been set. If it hasn't, Disposing the CacheEntry won't add it to the cache.

Fix dotnet#42321

src/libraries/Microsoft.Extensions.Caching.Abstractions/src/MemoryCacheExtensions.cs

@@ -43,39 +43,43 @@ public static bool TryGetValue<TItem>(this IMemoryCache cache, object key, out T
 
         public static TItem Set<TItem>(this IMemoryCache cache, object key, TItem value)
         {
-            ICacheEntry entry = cache.CreateEntry(key);
-            entry.Value = value;
-            entry.Dispose();
+            using (ICacheEntry entry = cache.CreateEntry(key))
+            {
+                entry.Value = value;
+            }
 
             return value;
         }
 
         public static TItem Set<TItem>(this IMemoryCache cache, object key, TItem value, DateTimeOffset absoluteExpiration)
         {
-            ICacheEntry entry = cache.CreateEntry(key);
-            entry.AbsoluteExpiration = absoluteExpiration;
-            entry.Value = value;
-            entry.Dispose();
+            using (ICacheEntry entry = cache.CreateEntry(key))
+            {
+                entry.AbsoluteExpiration = absoluteExpiration;
+                entry.Value = value;
+            }
 
             return value;
         }
 
         public static TItem Set<TItem>(this IMemoryCache cache, object key, TItem value, TimeSpan absoluteExpirationRelativeToNow)
         {
-            ICacheEntry entry = cache.CreateEntry(key);
-            entry.AbsoluteExpirationRelativeToNow = absoluteExpirationRelativeToNow;
-            entry.Value = value;
-            entry.Dispose();
+            using (ICacheEntry entry = cache.CreateEntry(key))
+            {
+                entry.AbsoluteExpirationRelativeToNow = absoluteExpirationRelativeToNow;
+                entry.Value = value;
+            }
 
             return value;
         }
 
         public static TItem Set<TItem>(this IMemoryCache cache, object key, TItem value, IChangeToken expirationToken)
         {
-            ICacheEntry entry = cache.CreateEntry(key);
-            entry.AddExpirationToken(expirationToken);
-            entry.Value = value;
-            entry.Dispose();
+            using (ICacheEntry entry = cache.CreateEntry(key))
+            {
+                entry.AddExpirationToken(expirationToken);
+                entry.Value = value;
+            }
 
             return value;
         }
@@ -99,13 +103,11 @@ public static TItem GetOrCreate<TItem>(this IMemoryCache cache, object key, Func
         {
             if (!cache.TryGetValue(key, out object result))
             {
-                ICacheEntry entry = cache.CreateEntry(key);
-                result = factory(entry);
-                entry.SetValue(result);
-                // need to manually call dispose instead of having a using
-                // in case the factory passed in throws, in which case we
-                // do not want to add the entry to the cache
-                entry.Dispose();
+                using (ICacheEntry entry = cache.CreateEntry(key))
+                {
+                    result = factory(entry);
+                    entry.SetValue(result);
+                }
             }
 
             return (TItem)result;
@@ -115,13 +117,11 @@ public static async Task<TItem> GetOrCreateAsync<TItem>(this IMemoryCache cache,
         {
             if (!cache.TryGetValue(key, out object result))
             {
-                ICacheEntry entry = cache.CreateEntry(key);
-                result = await factory(entry).ConfigureAwait(false);
-                entry.SetValue(result);
-                // need to manually call dispose instead of having a using
-                // in case the factory passed in throws, in which case we
-                // do not want to add the entry to the cache
-                entry.Dispose();
+                using (ICacheEntry entry = cache.CreateEntry(key))
+                {
+                    result = await factory(entry).ConfigureAwait(false);
+                    entry.SetValue(result);
+                }
             }
 
             return (TItem)result;

src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs

@@ -27,6 +27,7 @@ internal class CacheEntry : ICacheEntry
         private TimeSpan? _slidingExpiration;
         private long? _size;
         private IDisposable _scope;
+        private object _value;
 
         internal readonly object _lock = new object();
 
@@ -182,7 +183,17 @@ public long? Size
 
         public object Key { get; private set; }
 
-        public object Value { get; set; }
+        public object Value
+        {
+            get => _value;
+            set
+            {
+                _value = value;
+                ValueHasBeenSet = true;
+            }
+        }
+
+        internal bool ValueHasBeenSet { get; private set; }
 
         internal DateTimeOffset LastAccessed { get; set; }
 

src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs

@@ -116,6 +116,13 @@ private void SetEntry(CacheEntry entry)
                 return;
             }
 
+            if (!entry.ValueHasBeenSet)
+            {
+                // No-op if the CacheEntry Value was never set. We assume an exception occurred and the caller
+                // never set the Value successfully, so don't use this entry.
+                return;
+            }
+
             if (_options.SizeLimit.HasValue && !entry.Size.HasValue)
             {
                 throw new InvalidOperationException($"Cache entry must specify a value for {nameof(entry.Size)} when {nameof(_options.SizeLimit)} is set.");

src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs

@@ -180,6 +180,9 @@ public void GetOrCreate_WillNotCreateEmptyValue_WhenFactoryThrows()
             }
 
             Assert.False(cache.TryGetValue(key, out int obj));
+
+            // verify that throwing an exception doesn't leak CacheEntry objects
+            Assert.Null(CacheEntryHelper.Current);
         }
 
         [Fact]
@@ -199,6 +202,9 @@ await cache.GetOrCreateAsync<int>(key, entry =>
             }
 
             Assert.False(cache.TryGetValue(key, out int obj));
+
+            // verify that throwing an exception doesn't leak CacheEntry objects
+            Assert.Null(CacheEntryHelper.Current);
         }
 
         [Fact]