-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Async work started in ICacheEntry scope can keep the entry alive longer than needed #42992
Comments
Tagging subscribers to this area: @eerhardt, @maryamariyan |
This may have gotten fixed with #42355. We should investigate if it did. |
After reading the #42355 changes, I don't believe they fixed this. |
Such a fix is no longer possible after #45563 removed |
#45592 would let an application developer disable the implicit linking of cache entries and thereby avoid this problem. However:
|
The The original code is here; I'm trying to build an async cache that stores [Fact]
public async Task MemoryCache_FailureObserved()
{
var cache = new MemoryCache(new MemoryCacheOptions());
TaskCompletionSource<int> tcs = new();
CancellationTokenSource cts = new();
var entry = cache.CreateEntry("key");
entry.Value = tcs;
entry.AddExpirationToken(new CancellationChangeToken(cts.Token));
entry.Dispose();
InvokeAndPropagateCompletion(cache.CreateEntry("key"), tcs);
var task = tcs.Task;
await task;
cache.TryGetValue("key", out _);
#pragma warning disable 1998
async void InvokeAndPropagateCompletion(ICacheEntry cacheEntry, TaskCompletionSource<int> tcs)
#pragma warning restore 1998
{
if (cache.TryGetValue(cacheEntry.Key, out TaskCompletionSource<int> existingTcs) && existingTcs == tcs)
{
using (cacheEntry)
cacheEntry.Value = tcs;
}
tcs.TrySetResult(13);
}
} I find the implicit behavior surprising. I consider it a pit of failure, and I am in agreement with @davidfowl that linked cache entries should be off by default. Perhaps I'm missing something, but are "linked cache entries" documented anywhere? In the meantime, I've adopted the workaround below, which works if you replace every instance of internal sealed class SafeCacheEntry : ICacheEntry
{
private readonly ICacheEntry _cacheEntryImplementation;
private SafeCacheEntry(ICacheEntry cacheEntryImplementation) => _cacheEntryImplementation = cacheEntryImplementation;
public static ICacheEntry Create(IMemoryCache cache, object key)
{
return AsyncCreateEntry().GetAwaiter().GetResult();
#pragma warning disable 1998
async Task<ICacheEntry> AsyncCreateEntry() => new SafeCacheEntry(cache.CreateEntry(key));
#pragma warning restore 1998
}
public void Dispose()
{
AsyncDispose().GetAwaiter().GetResult();
#pragma warning disable 1998
async Task AsyncDispose() => _cacheEntryImplementation.Dispose();
#pragma warning restore 1998
}
public object Key => _cacheEntryImplementation.Key;
public object Value
{
get => _cacheEntryImplementation.Value;
set => _cacheEntryImplementation.Value = value;
}
public DateTimeOffset? AbsoluteExpiration
{
get => _cacheEntryImplementation.AbsoluteExpiration;
set => _cacheEntryImplementation.AbsoluteExpiration = value;
}
public TimeSpan? AbsoluteExpirationRelativeToNow
{
get => _cacheEntryImplementation.AbsoluteExpirationRelativeToNow;
set => _cacheEntryImplementation.AbsoluteExpirationRelativeToNow = value;
}
public TimeSpan? SlidingExpiration
{
get => _cacheEntryImplementation.SlidingExpiration;
set => _cacheEntryImplementation.SlidingExpiration = value;
}
public IList<IChangeToken> ExpirationTokens => _cacheEntryImplementation.ExpirationTokens;
public IList<PostEvictionCallbackRegistration> PostEvictionCallbacks => _cacheEntryImplementation.PostEvictionCallbacks;
public CacheItemPriority Priority
{
get => _cacheEntryImplementation.Priority;
set => _cacheEntryImplementation.Priority = value;
}
public long? Size
{
get => _cacheEntryImplementation.Size;
set => _cacheEntryImplementation.Size = value;
}
} This wrapper type works by forcing the |
They are since .NET 7: #57648. Since this almost undocumented feature is not enabled by default anymore, I really doubt we are ever going to spend time working on a fix. Having said that, I am going to close the issue. If you still believe that it's important please send a PR with a fix, I would be more than happy to review it and help with the benchmarking. |
How about a doc change at MemoryCacheOptions.TrackLinkedCacheEntries, warning about risks if the developer enables this option again? |
Describe the bug
If a method creates an
ICacheEntry
and then starts asynchronous work (e.g. task, timer, or thread) before it commits the entry to a cache (by callingDispose
), then the asynchronous work inherits an execution context in whichCacheEntryHelper._scopes
indirectly refers to the entry. This reference can prevent the cache entry from being collected as garbage, even after the entry has been committed to the cache and expired from the cache.Furthermore, if the asynchronous work creates its own cache entries and adds expiration tokens to them, then those expiration tokens propagate to the outer cache entry, even though the outer cache entry was already assigned a value and committed. This prevents the expiration tokens from being collected as garbage and may cause the outer cache entry to expire prematurely.
To Reproduce
Run the following test against version 2.1.2 of package Microsoft.Extensions.Caching.Memory on .NET Framework 4.8. This code uses
CreateCache
andTestExpirationToken
from the existing tests of the package.Expected behavior
The test passes.
Actual behavior
The test fails at
Assert.Null(CacheEntryHelper.Current)
ifMEMORY_CACHE_INTERNAL
is defined, or atAssert.Empty(parentEntry.ExpirationTokens)
otherwise.Additional context
A fix might involve assigning
CacheEntryStack._entry = null
duringCacheEntry.Dispose
. That way, if aCacheEntryStack
is left in some execution contexts afterCacheEntry.Dispose
, it would no longer cause expiration options to be propagated to theCacheEntry
and would not prevent it from being collected. If theCacheEntryStack._previous
field were also deleted (it is only read by debuggers), then execution contexts would not be able to accumulate a chain of staleCacheEntryStack
instances, either.Alternatively, one could document the issue and recommend using
ExecutionContext.SuppressFlow
around async operations that are started within the scope of a cache entry.Originally noted in dotnet/extensions#3533 (comment)
The text was updated successfully, but these errors were encountered: