Skip to content

Commit 5dc3e89

Browse files
wtgodbewtgodbe
authored
[release/3.1] Memory Leak in Microsoft.Extensions.Caching.Memory when handling exceptions (#3535)
* Memory Leak in Microsoft.Extensions.Caching.Memory when handling exceptions * Fixup * Fixup * Fix another memory leak when one cache depends on another cache
1 parent 072601d commit 5dc3e89

File tree

3 files changed

+87
-40
lines changed

3 files changed

+87
-40
lines changed

‎src/Caching/Abstractions/src/MemoryCacheExtensions.cs‎

Lines changed: 29 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -43,39 +43,43 @@ public static bool TryGetValue<TItem>(this IMemoryCache cache, object key, out T
4343

4444
public static TItem Set<TItem>(this IMemoryCache cache, object key, TItem value)
4545
{
46-
var entry = cache.CreateEntry(key);
47-
entry.Value = value;
48-
entry.Dispose();
46+
using (var entry = cache.CreateEntry(key))
47+
{
48+
entry.Value = value;
49+
}
4950

5051
return value;
5152
}
5253

5354
public static TItem Set<TItem>(this IMemoryCache cache, object key, TItem value, DateTimeOffset absoluteExpiration)
5455
{
55-
var entry = cache.CreateEntry(key);
56-
entry.AbsoluteExpiration = absoluteExpiration;
57-
entry.Value = value;
58-
entry.Dispose();
56+
using (var entry = cache.CreateEntry(key))
57+
{
58+
entry.AbsoluteExpiration = absoluteExpiration;
59+
entry.Value = value;
60+
}
5961

6062
return value;
6163
}
6264

6365
public static TItem Set<TItem>(this IMemoryCache cache, object key, TItem value, TimeSpan absoluteExpirationRelativeToNow)
6466
{
65-
var entry = cache.CreateEntry(key);
66-
entry.AbsoluteExpirationRelativeToNow = absoluteExpirationRelativeToNow;
67-
entry.Value = value;
68-
entry.Dispose();
67+
using (var entry = cache.CreateEntry(key))
68+
{
69+
entry.AbsoluteExpirationRelativeToNow = absoluteExpirationRelativeToNow;
70+
entry.Value = value;
71+
}
6972

7073
return value;
7174
}
7275

7376
public static TItem Set<TItem>(this IMemoryCache cache, object key, TItem value, IChangeToken expirationToken)
7477
{
75-
var entry = cache.CreateEntry(key);
76-
entry.AddExpirationToken(expirationToken);
77-
entry.Value = value;
78-
entry.Dispose();
78+
using (var entry = cache.CreateEntry(key))
79+
{
80+
entry.AddExpirationToken(expirationToken);
81+
entry.Value = value;
82+
}
7983

8084
return value;
8185
}
@@ -99,13 +103,11 @@ public static TItem GetOrCreate<TItem>(this IMemoryCache cache, object key, Func
99103
{
100104
if (!cache.TryGetValue(key, out object result))
101105
{
102-
var entry = cache.CreateEntry(key);
103-
result = factory(entry);
104-
entry.SetValue(result);
105-
// need to manually call dispose instead of having a using
106-
// in case the factory passed in throws, in which case we
107-
// do not want to add the entry to the cache
108-
entry.Dispose();
106+
using (var entry = cache.CreateEntry(key))
107+
{
108+
result = factory(entry);
109+
entry.Value = result;
110+
}
109111
}
110112

111113
return (TItem)result;
@@ -115,13 +117,11 @@ public static async Task<TItem> GetOrCreateAsync<TItem>(this IMemoryCache cache,
115117
{
116118
if (!cache.TryGetValue(key, out object result))
117119
{
118-
var entry = cache.CreateEntry(key);
119-
result = await factory(entry);
120-
entry.SetValue(result);
121-
// need to manually call dispose instead of having a using
122-
// in case the factory passed in throws, in which case we
123-
// do not want to add the entry to the cache
124-
entry.Dispose();
120+
using (ICacheEntry entry = cache.CreateEntry(key))
121+
{
122+
result = await factory(entry).ConfigureAwait(false);
123+
entry.Value = result;
124+
}
125125
}
126126

127127
return (TItem)result;

‎src/Caching/Memory/src/CacheEntry.cs‎

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@ namespace Microsoft.Extensions.Caching.Memory
1111
{
1212
internal class CacheEntry : ICacheEntry
1313
{
14-
private bool _added = false;
14+
private bool _disposed = false;
1515
private static readonly Action<object> ExpirationCallback = ExpirationTokensExpired;
1616
private readonly Action<CacheEntry> _notifyCacheOfExpiration;
17-
private readonly Action<CacheEntry> _notifyCacheEntryDisposed;
17+
private readonly Action<CacheEntry> _notifyCacheEntryCommit;
1818
private IList<IDisposable> _expirationTokenRegistrations;
1919
private IList<PostEvictionCallbackRegistration> _postEvictionCallbacks;
2020
private bool _isExpired;
@@ -25,22 +25,24 @@ internal class CacheEntry : ICacheEntry
2525
private TimeSpan? _slidingExpiration;
2626
private long? _size;
2727
private IDisposable _scope;
28+
private object _value;
29+
private bool _valueHasBeenSet;
2830

2931
internal readonly object _lock = new object();
3032

3133
internal CacheEntry(
3234
object key,
33-
Action<CacheEntry> notifyCacheEntryDisposed,
35+
Action<CacheEntry> notifyCacheEntryCommit,
3436
Action<CacheEntry> notifyCacheOfExpiration)
3537
{
3638
if (key == null)
3739
{
3840
throw new ArgumentNullException(nameof(key));
3941
}
4042

41-
if (notifyCacheEntryDisposed == null)
43+
if (notifyCacheEntryCommit == null)
4244
{
43-
throw new ArgumentNullException(nameof(notifyCacheEntryDisposed));
45+
throw new ArgumentNullException(nameof(notifyCacheEntryCommit));
4446
}
4547

4648
if (notifyCacheOfExpiration == null)
@@ -49,7 +51,7 @@ internal CacheEntry(
4951
}
5052

5153
Key = key;
52-
_notifyCacheEntryDisposed = notifyCacheEntryDisposed;
54+
_notifyCacheEntryCommit = notifyCacheEntryCommit;
5355
_notifyCacheOfExpiration = notifyCacheOfExpiration;
5456

5557
_scope = CacheEntryHelper.EnterScope(this);
@@ -173,20 +175,39 @@ public long? Size
173175

174176
public object Key { get; private set; }
175177

176-
public object Value { get; set; }
178+
public object Value
179+
{
180+
get => _value;
181+
set
182+
{
183+
_value = value;
184+
_valueHasBeenSet = true;
185+
}
186+
}
177187

178188
internal DateTimeOffset LastAccessed { get; set; }
179189

180190
internal EvictionReason EvictionReason { get; private set; }
181191

182192
public void Dispose()
183193
{
184-
if (!_added)
194+
if (!_disposed)
185195
{
186-
_added = true;
196+
_disposed = true;
197+
198+
// Ensure the _scope reference is cleared because it can reference other CacheEntry instances.
199+
// This CacheEntry is going to be put into a MemoryCache, and we don't want to root unnecessary objects.
187200
_scope.Dispose();
188-
_notifyCacheEntryDisposed(this);
189-
PropagateOptions(CacheEntryHelper.Current);
201+
_scope = null;
202+
203+
// Don't commit or propagate options if the CacheEntry Value was never set.
204+
// We assume an exception occurred causing the caller to not set the Value successfully,
205+
// so don't use this entry.
206+
if (_valueHasBeenSet)
207+
{
208+
_notifyCacheEntryCommit(this);
209+
PropagateOptions(CacheEntryHelper.Current);
210+
}
190211
}
191212
}
192213

‎src/Caching/Memory/test/MemoryCacheSetAndRemoveTests.cs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5+
using System.Reflection;
56
using System.Threading;
67
using System.Threading.Tasks;
78
using Xunit;
@@ -180,6 +181,9 @@ public void GetOrCreate_WillNotCreateEmptyValue_WhenFactoryThrows()
180181
}
181182

182183
Assert.False(cache.TryGetValue(key, out int obj));
184+
185+
// verify that throwing an exception doesn't leak CacheEntry objects
186+
Assert.Null(CacheEntryHelper.Current);
183187
}
184188

185189
[Fact]
@@ -199,6 +203,28 @@ await cache.GetOrCreateAsync<int>(key, entry =>
199203
}
200204

201205
Assert.False(cache.TryGetValue(key, out int obj));
206+
207+
// verify that throwing an exception doesn't leak CacheEntry objects
208+
Assert.Null(CacheEntryHelper.Current);
209+
}
210+
211+
[Fact]
212+
public void DisposingCacheEntryReleasesScope()
213+
{
214+
object GetScope(ICacheEntry entry)
215+
{
216+
return entry.GetType()
217+
.GetField("_scope", BindingFlags.NonPublic | BindingFlags.Instance)
218+
.GetValue(entry);
219+
}
220+
221+
var cache = CreateCache();
222+
223+
ICacheEntry entry = cache.CreateEntry("myKey");
224+
Assert.NotNull(GetScope(entry));
225+
226+
entry.Dispose();
227+
Assert.Null(GetScope(entry));
202228
}
203229

204230
[Fact]

0 commit comments

Comments
 (0)