diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 78f24d40e0e..884b94cbe5e 100644 --- a/eng/PatchConfig.props +++ b/eng/PatchConfig.props @@ -13,6 +13,8 @@ + Microsoft.Extensions.Caching.Abstractions; + Microsoft.Extensions.Caching.Memory; diff --git a/src/Caching/Abstractions/src/MemoryCacheExtensions.cs b/src/Caching/Abstractions/src/MemoryCacheExtensions.cs index e415d27844f..c0ba3af90ed 100644 --- a/src/Caching/Abstractions/src/MemoryCacheExtensions.cs +++ b/src/Caching/Abstractions/src/MemoryCacheExtensions.cs @@ -35,39 +35,43 @@ public static bool TryGetValue(this IMemoryCache cache, object key, out T public static TItem Set(this IMemoryCache cache, object key, TItem value) { - var entry = cache.CreateEntry(key); - entry.Value = value; - entry.Dispose(); + using (var entry = cache.CreateEntry(key)) + { + entry.Value = value; + } return value; } public static TItem Set(this IMemoryCache cache, object key, TItem value, DateTimeOffset absoluteExpiration) { - var entry = cache.CreateEntry(key); - entry.AbsoluteExpiration = absoluteExpiration; - entry.Value = value; - entry.Dispose(); + using (var entry = cache.CreateEntry(key)) + { + entry.AbsoluteExpiration = absoluteExpiration; + entry.Value = value; + } return value; } public static TItem Set(this IMemoryCache cache, object key, TItem value, TimeSpan absoluteExpirationRelativeToNow) { - var entry = cache.CreateEntry(key); - entry.AbsoluteExpirationRelativeToNow = absoluteExpirationRelativeToNow; - entry.Value = value; - entry.Dispose(); + using (var entry = cache.CreateEntry(key)) + { + entry.AbsoluteExpirationRelativeToNow = absoluteExpirationRelativeToNow; + entry.Value = value; + } return value; } public static TItem Set(this IMemoryCache cache, object key, TItem value, IChangeToken expirationToken) { - var entry = cache.CreateEntry(key); - entry.AddExpirationToken(expirationToken); - entry.Value = value; - entry.Dispose(); + using (var entry = cache.CreateEntry(key)) + { + entry.AddExpirationToken(expirationToken); + entry.Value = value; + } return value; } @@ -91,13 +95,11 @@ public static TItem GetOrCreate(this IMemoryCache cache, object key, Func { if (!cache.TryGetValue(key, out object result)) { - var 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 (var entry = cache.CreateEntry(key)) + { + result = factory(entry); + entry.Value = result; + } } return (TItem)result; @@ -107,13 +109,11 @@ public static async Task GetOrCreateAsync(this IMemoryCache cache, { if (!cache.TryGetValue(key, out object result)) { - var entry = cache.CreateEntry(key); - result = await 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 = await factory(entry).ConfigureAwait(false); + entry.Value = result; + } } return (TItem)result; diff --git a/src/Caching/Memory/src/CacheEntry.cs b/src/Caching/Memory/src/CacheEntry.cs index 17dda76bf61..fb0a3717615 100644 --- a/src/Caching/Memory/src/CacheEntry.cs +++ b/src/Caching/Memory/src/CacheEntry.cs @@ -11,10 +11,10 @@ namespace Microsoft.Extensions.Caching.Memory { internal class CacheEntry : ICacheEntry { - private bool _added = false; + private bool _disposed = false; private static readonly Action ExpirationCallback = ExpirationTokensExpired; private readonly Action _notifyCacheOfExpiration; - private readonly Action _notifyCacheEntryDisposed; + private readonly Action _notifyCacheEntryCommit; private IList _expirationTokenRegistrations; private IList _postEvictionCallbacks; private bool _isExpired; @@ -25,12 +25,14 @@ internal class CacheEntry : ICacheEntry private TimeSpan? _slidingExpiration; private long? _size; private IDisposable _scope; + private object _value; + private bool _valueHasBeenSet; internal readonly object _lock = new object(); internal CacheEntry( object key, - Action notifyCacheEntryDisposed, + Action notifyCacheEntryCommit, Action notifyCacheOfExpiration) { if (key == null) @@ -38,9 +40,9 @@ internal CacheEntry( throw new ArgumentNullException(nameof(key)); } - if (notifyCacheEntryDisposed == null) + if (notifyCacheEntryCommit == null) { - throw new ArgumentNullException(nameof(notifyCacheEntryDisposed)); + throw new ArgumentNullException(nameof(notifyCacheEntryCommit)); } if (notifyCacheOfExpiration == null) @@ -49,7 +51,7 @@ internal CacheEntry( } Key = key; - _notifyCacheEntryDisposed = notifyCacheEntryDisposed; + _notifyCacheEntryCommit = notifyCacheEntryCommit; _notifyCacheOfExpiration = notifyCacheOfExpiration; _scope = CacheEntryHelper.EnterScope(this); @@ -173,7 +175,15 @@ 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 DateTimeOffset LastAccessed { get; set; } @@ -181,12 +191,23 @@ public long? Size public void Dispose() { - if (!_added) + if (!_disposed) { - _added = true; + _disposed = true; + + // Ensure the _scope reference is cleared because it can reference other CacheEntry instances. + // This CacheEntry is going to be put into a MemoryCache, and we don't want to root unnecessary objects. _scope.Dispose(); - _notifyCacheEntryDisposed(this); - PropagateOptions(CacheEntryHelper.Current); + _scope = null; + + // Don't commit or propagate options if the CacheEntry Value was never set. + // We assume an exception occurred causing the caller to not set the Value successfully, + // so don't use this entry. + if (_valueHasBeenSet) + { + _notifyCacheEntryCommit(this); + PropagateOptions(CacheEntryHelper.Current); + } } } diff --git a/src/Caching/Memory/test/MemoryCacheSetAndRemoveTests.cs b/src/Caching/Memory/test/MemoryCacheSetAndRemoveTests.cs index 5136f1ed3ce..3dae5e31aa2 100644 --- a/src/Caching/Memory/test/MemoryCacheSetAndRemoveTests.cs +++ b/src/Caching/Memory/test/MemoryCacheSetAndRemoveTests.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Reflection; using System.Threading; using System.Threading.Tasks; using Xunit; @@ -180,6 +181,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 +203,21 @@ await cache.GetOrCreateAsync(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] + public void DisposingCacheEntryReleasesScope() + { + var cache = CreateCache(); + + ICacheEntry entry = cache.CreateEntry("myKey"); + Assert.NotNull(GetScope(entry)); + + entry.Dispose(); + Assert.Null(GetScope(entry)); } [Fact] @@ -625,6 +644,13 @@ public async Task GetOrCreateAsyncFromCacheWithNullKeyThrows() await Assert.ThrowsAsync(async () => await cache.GetOrCreateAsync(null, null)); } + private object GetScope(ICacheEntry entry) + { + return entry.GetType() + .GetField("_scope", BindingFlags.NonPublic | BindingFlags.Instance) + .GetValue(entry); + } + private class TestKey { public override bool Equals(object obj) => true; diff --git a/src/Caching/Memory/test/Microsoft.Extensions.Caching.Memory.Tests.csproj b/src/Caching/Memory/test/Microsoft.Extensions.Caching.Memory.Tests.csproj index 4a96edcb5ca..081022d64a5 100644 --- a/src/Caching/Memory/test/Microsoft.Extensions.Caching.Memory.Tests.csproj +++ b/src/Caching/Memory/test/Microsoft.Extensions.Caching.Memory.Tests.csproj @@ -6,6 +6,7 @@ +