GetOrCreate/GetOrCreateAsync may lead to memory leaks?

[gravity00]

While analyzing some memory leaks, while trying to reduce closures to the minimum, I may have found a bug with the extensions GetOrCreate and GetOrCreateAsync. Because the current design requires the entry to be disposed so the value would be added to the cache, it can't implement the disposable pattern as expected, by invoking the Dispose in the finalizer. The problem is that it hold references for other disposable instances and, at least the GetOrCreate methods, may have some unexpected behavior. Lets imagine the following code:

await cache.GetOrCreateAsync("potatoes", async k => {
    var potatoes = // get value from database 
    if (potatoes == null)
        thrown new Exception("Value is null");
    return potatoes;
});

When looking at the implementation and comments, the expected behavior is not to add the value in case of an exception, but the problem is that an entry will be created but the dispose won't be called if an exception is thrown:

https://github.com/aspnet/Caching/blob/56447f941b39337947273476b2c366b3dffde565/src/Microsoft.Extensions.Caching.Abstractions/MemoryCacheExtensions.cs#L92-L121

Because removing the dispose now would be a breaking change (#340), my recommendation to patch this bug is to invoke the factory before creating the entry with new overloads and obsolete the current ones. This would ensure that, in case of an exception, the entry wouldn't be created and nothing would be added to the cache, as expected. Something as follows:

public static TItem GetOrCreate<TItem>(this IMemoryCache cache, object key, Func<TItem> factory, Action<ICacheEntry> config = null)
{
	if (!cache.TryGetValue(key, out object result))
	{
                result = factory();
		using(var entry = cache.CreateEntry(key))
                {
                        entry.SetValue(result);
                        config?.Invoke(entry);
                }
	}

	return (TItem)result;
}

[KevinCathcart]

The code as-is is almost correct, at least with respect to the actual implementation. Not calling the dispose on CacheEntry would be fine if it were not for the CacheEntryStack. If that were not present, nothing would actually get leaked.

For some background, what the CacheEntryStack system does is cause any new cache entry to expire when any of the entries it used in its delegate expire. that is to say in the following, "a" will expire when "b" expires.

memCache.GetOrCreate("a", _ => 2 * memCache.Get<int>("b") + 5);

This whole system means that gravity00's suggestion won't fix this correctly.

Of course, the whole EntryCacheStack breaks if cache entry delegate throws. Consider the following:

memCache.GetOrCreate("a", _ =>
{
	int c;
	try
	{
		c = memCache.GetOrCreate(
                    $"int_value:{someFilePath}", 
                    __ => int.Parse(File.ReadAllText(someFilePath)));
	}
	catch(FileNotFoundException)
	{
		// File not found is expected, since 
                // the file will not always be present. 
                // Use a default of 15 in this case.
		c=15;
	}
	var b=memCache.Get<int>("b");

	return b+c;
});

in this case "a" will not inherit "b"'s cache expiration because the EntryCacheStack was messed up when "c" threw. Now obviously that code as shown is not something somebody would actually write, but it is not unreasonable for the basic structure to occur. The file read caching might be deeply nested in some complex business process, with specific expected exception types only handled at a higher level.

So this is not only a memory leak, but a correctness bug as well, since expiration of "a" upon expiration of "b" is documented behavior, but throwing an exception while trying to cache the file read caused that behavior to be violated.

[gravity00]

@KevinCathcart yes, you are correct. I didn't know about that nested behavior at the time.

I also didn't went into more details about how using Dispose to commit is not a great idea because there was already a related issue for that: https://github.com/aspnet/Extensions/issues/719
It seems in version 3 they made a work around so no breaking changes will be made but I'm still very reluctant about that.

[eerhardt]

This looks like it was fixed with #42355. We should investigate if it did.