Memory Leak in Microsoft.Extensions.Caching.Memory when handling exceptions

[eerhardt]

We are leaking objects when calling MemoryCache.GetOrCreate and the factory method is throwing an exception.

Repro

Run the following application, take a memory snapshot, run for a while and take another snapshot

using Microsoft.Extensions.Caching.Memory;
using System;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            var cache = new MemoryCache(new MemoryCacheOptions());
            while (true)
            {
                try
                {
                    int value = cache.GetOrCreate("hello", entry =>
                    {
                        throw new Exception();
                        return 10;
                    });
                }
                catch
                {
                    Console.WriteLine("Get Failed");
                }
            }
        }
    }
}

Analysis

It appears the issue is that we aren't Disposing the CacheEntry instances when an exception is being thrown:

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

Lines 98 to 112 in 33dba95

	public static TItem GetOrCreate<TItem>(this IMemoryCache cache, object key, Func<ICacheEntry, TItem> factory)
	{
	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();
	}
	return (TItem)result;
	}

The reason we aren't calling Dispose is to fix an issue that we were caching null when an Exception was being thrown. See aspnet/Caching#216.

Disposing the CacheEntry is important because every time you create a CacheEntry, it gets stuck in an AsyncLocal "stack":

runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs

Lines 28 to 36 in 33dba95

	internal static IDisposable EnterScope(CacheEntry entry)
	{
	CacheEntryStack scopes = GetOrCreateScopes();
	var scopeLease = new ScopeLease(scopes);
	Scopes = scopes.Push(entry);
	return scopeLease;
	}

and disposing it is what "pops" it off the stack:

runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs

Lines 50 to 63 in 33dba95

	private sealed class ScopeLease : IDisposable
	{
	private readonly CacheEntryStack _cacheEntryStack;
	public ScopeLease(CacheEntryStack cacheEntryStack)
	{
	_cacheEntryStack = cacheEntryStack;
	}
	public void Dispose()
	{
	Scopes = _cacheEntryStack;
	}
	}

We should always be Disposing the entry. That way the ScopeLease object is always disposed, and the stack is cleared.

However, we still need to fix the original problem: Don't cache null when an Exception happens. The reason this happens is because Disposing the CacheEntry is what "commits the entry into the cache". This method gets called from CacheEntry.Dispose:

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

Lines 111 to 234 in 33dba95

	private void SetEntry(CacheEntry entry)
	{
	if (_disposed)
	{
	// No-op instead of throwing since this is called during CacheEntry.Dispose
	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.");
	}
	DateTimeOffset utcNow = _options.Clock.UtcNow;
	DateTimeOffset? absoluteExpiration = null;
	if (entry._absoluteExpirationRelativeToNow.HasValue)
	{
	absoluteExpiration = utcNow + entry._absoluteExpirationRelativeToNow;
	}
	else if (entry._absoluteExpiration.HasValue)
	{
	absoluteExpiration = entry._absoluteExpiration;
	}
	// Applying the option's absolute expiration only if it's not already smaller.
	// This can be the case if a dependent cache entry has a smaller value, and
	// it was set by cascading it to its parent.
	if (absoluteExpiration.HasValue)
	{
	if (!entry._absoluteExpiration.HasValue || absoluteExpiration.Value < entry._absoluteExpiration.Value)
	{
	entry._absoluteExpiration = absoluteExpiration;
	}
	}
	// Initialize the last access timestamp at the time the entry is added
	entry.LastAccessed = utcNow;
	if (_entries.TryGetValue(entry.Key, out CacheEntry priorEntry))
	{
	priorEntry.SetExpired(EvictionReason.Replaced);
	}
	bool exceedsCapacity = UpdateCacheSizeExceedsCapacity(entry);
	if (!entry.CheckExpired(utcNow) && !exceedsCapacity)
	{
	bool entryAdded = false;
	if (priorEntry == null)
	{
	// Try to add the new entry if no previous entries exist.
	entryAdded = _entries.TryAdd(entry.Key, entry);
	}
	else
	{
	// Try to update with the new entry if a previous entries exist.
	entryAdded = _entries.TryUpdate(entry.Key, entry, priorEntry);
	if (entryAdded)
	{
	if (_options.SizeLimit.HasValue)
	{
	// The prior entry was removed, decrease the by the prior entry's size
	Interlocked.Add(ref _cacheSize, -priorEntry.Size.Value);
	}
	}
	else
	{
	// The update will fail if the previous entry was removed after retrival.
	// Adding the new entry will succeed only if no entry has been added since.
	// This guarantees removing an old entry does not prevent adding a new entry.
	entryAdded = _entries.TryAdd(entry.Key, entry);
	}
	}
	if (entryAdded)
	{
	entry.AttachTokens();
	}
	else
	{
	if (_options.SizeLimit.HasValue)
	{
	// Entry could not be added, reset cache size
	Interlocked.Add(ref _cacheSize, -entry.Size.Value);
	}
	entry.SetExpired(EvictionReason.Replaced);
	entry.InvokeEvictionCallbacks();
	}
	if (priorEntry != null)
	{
	priorEntry.InvokeEvictionCallbacks();
	}
	}
	else
	{
	if (exceedsCapacity)
	{
	// The entry was not added due to overcapacity
	entry.SetExpired(EvictionReason.Capacity);
	TriggerOvercapacityCompaction();
	}
	else
	{
	if (_options.SizeLimit.HasValue)
	{
	// Entry could not be added due to being expired, reset cache size
	Interlocked.Add(ref _cacheSize, -entry.Size.Value);
	}
	}
	entry.InvokeEvictionCallbacks();
	if (priorEntry != null)
	{
	RemoveEntry(priorEntry);
	}
	}
	StartScanForExpiredItems(utcNow);
	}

To fix this, we should set a flag indicating whether the CacheEntry.Value was ever set. If it wasn't set, we shouldn't be committing the value into the cache.

cc @Tratcher

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

[Tratcher]

Design hindsight: Dispose was the wrong term, it should have been named Commit or similar. Then Dispose could have been used for general cleanup without committing.

To fix this, we should set a flag indicating whether the CacheEntry.Value was ever set. If it wasn't set, we shouldn't be committing the value into the cache.

👍

There's still some risk of an exception being thrown after the value was set, such as when setting up the change tokens, timeouts, etc..

[eerhardt]

Design hindsight: Dispose was the wrong term, it should have been named Commit or similar. Then Dispose could have been used for general cleanup without committing.

+💯

There's still some risk of an exception being thrown after the value was set, such as when setting up the change tokens, timeouts, etc..

Agreed, but that is more of an edge case. That won't happen when using the GetOrCreate methods, only if someone is calling CreateEntry themselves. To fix that, they can move SetValue to the last thing before calling Dispose.

One other risk is that if someone is relying on the behavior of not calling SetValue to mean "cache null". In that case, they would need to start calling SetValue(null). But I think that is much less common than the mainline here.

[eerhardt]

Reopening to back port to 5.0.

[eerhardt]

Closing as the fix is now in the 5.0 branch.