Skip to content

Commit a660c0f

Browse files
danmoseleydanmoseley
authored
Fix MemoryCache test failures due to race (#72821)
1 parent ed2a5a1 commit a660c0f

File tree

4 files changed

+78
-50
lines changed

4 files changed

+78
-50
lines changed

‎src/libraries/Microsoft.Extensions.Caching.Abstractions/src/ICacheEntry.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ namespace Microsoft.Extensions.Caching.Memory
99
{
1010
/// <summary>
1111
/// Represents an entry in the <see cref="IMemoryCache"/> implementation.
12+
/// When Disposed, is committed to the cache.
1213
/// </summary>
1314
public interface ICacheEntry : IDisposable
1415
{

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

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,12 @@ public MemoryCache(IOptions<MemoryCacheOptions> optionsAccessor, ILoggerFactory
7676
/// </summary>
7777
public int Count => _coherentState.Count;
7878

79-
// internal for testing
79+
/// <summary>
80+
/// Internal accessor for Size for testing only.
81+
///
82+
/// Note that this is only eventually consistent with the contents of the collection.
83+
/// See comment on <see cref="CoherentState"/>.
84+
/// </summary>
8085
internal long Size => _coherentState.Size;
8186

8287
internal bool TrackLinkedCacheEntries { get; }
@@ -421,6 +426,10 @@ private void ScanForExpiredItems()
421426
}
422427
}
423428

429+
/// <summary>
430+
/// Returns true if increasing the cache size by the size of entry would
431+
/// cause it to exceed any size limit on the cache, otherwise, returns false.
432+
/// </summary>
424433
private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry, CoherentState coherentState)
425434
{
426435
long sizeLimit = _options.SizeLimitValue;
@@ -613,6 +622,22 @@ private static void ValidateCacheKey(object key)
613622
ThrowHelper.ThrowIfNull(key);
614623
}
615624

625+
/// <summary>
626+
/// Wrapper for the memory cache entries collection.
627+
///
628+
/// Entries may have various sizes. If a size limit has been set, the cache keeps track of the aggregate of all the entries' sizes
629+
/// in order to trigger compaction when the size limit is exceeded.
630+
///
631+
/// For performance reasons, the size is not updated atomically with the collection, but is only made eventually consistent.
632+
///
633+
/// When the memory cache is cleared, it replaces the backing collection entirely. This may occur in parallel with operations
634+
/// like add, set, remove, and compact which may modify the collection and thus its overall size.
635+
///
636+
/// To keep the overall size eventually consistent, therefore, the collection and the overall size are wrapped in this CoherentState
637+
/// object. Individual operations take a local reference to this wrapper object while they work, and make size updates to this object.
638+
/// Clearing the cache simply replaces the object, so that any still in progress updates do not affect the overall size value for
639+
/// the new backing collection.
640+
/// </summary>
616641
private sealed class CoherentState
617642
{
618643
internal ConcurrentDictionary<object, CacheEntry> _entries = new ConcurrentDictionary<object, CacheEntry>();

‎src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs‎

Lines changed: 49 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -72,45 +72,44 @@ public void AddingEntryIncreasesCacheSizeWhenEnforcingSizeLimit()
7272
{
7373
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });
7474

75-
Assert.Equal(0, cache.Size);
75+
AssertCacheSize(0, cache);
7676

7777
cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 });
7878

79-
Assert.Equal(5, cache.Size);
79+
AssertCacheSize(5, cache);
8080
}
8181

8282
[Fact]
8383
public void AddingEntryDoesNotIncreasesCacheSizeWhenNotEnforcingSizeLimit()
8484
{
8585
var cache = new MemoryCache(new MemoryCacheOptions());
8686

87-
Assert.Equal(0, cache.Size);
87+
AssertCacheSize(0, cache);
8888

8989
cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 });
9090

91-
Assert.Equal(0, cache.Size);
91+
AssertCacheSize(0, cache);
9292
}
9393

9494
[Fact]
9595
public void DoNotAddEntryIfItExceedsCapacity()
9696
{
9797
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });
9898

99-
Assert.Equal(0, cache.Size);
99+
AssertCacheSize(0, cache);
100100

101101
cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 4 });
102102

103103
Assert.Equal("value", cache.Get("key"));
104-
Assert.Equal(4, cache.Size);
104+
AssertCacheSize(4, cache);
105105

106106
cache.Set("key2", "value2", new MemoryCacheEntryOptions { Size = 7 });
107107

108108
Assert.Null(cache.Get("key2"));
109-
Assert.Equal(4, cache.Size);
109+
AssertCacheSize(4, cache);
110110
}
111111

112112
[Fact]
113-
[ActiveIssue("https://github.com/dotnet/runtime/issues/33993")]
114113
public async Task DoNotAddIfSizeOverflows()
115114
{
116115
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = long.MaxValue });
@@ -123,12 +122,12 @@ public async Task DoNotAddIfSizeOverflows()
123122
State = null
124123
});
125124

126-
Assert.Equal(0, cache.Size);
125+
AssertCacheSize(0, cache);
127126

128127
cache.Set("key", "value", entryOptions);
129128

130129
Assert.Equal("value", cache.Get("key"));
131-
Assert.Equal(long.MaxValue, cache.Size);
130+
AssertCacheSize(long.MaxValue, cache);
132131

133132
cache.Set("key1", "value1", new MemoryCacheEntryOptions { Size = long.MaxValue });
134133
// Do not add the new item
@@ -139,11 +138,10 @@ public async Task DoNotAddIfSizeOverflows()
139138

140139
// Compaction removes old item
141140
Assert.Null(cache.Get("key"));
142-
Assert.Equal(0, cache.Size);
141+
AssertCacheSize(0, cache);
143142
}
144143

145144
[Fact]
146-
[ActiveIssue("https://github.com/dotnet/runtime/issues/33993")]
147145
public async Task ExceedsCapacityCompacts()
148146
{
149147
var cache = new MemoryCache(new MemoryCacheOptions
@@ -161,12 +159,12 @@ public async Task ExceedsCapacityCompacts()
161159
State = null
162160
});
163161

164-
Assert.Equal(0, cache.Size);
162+
AssertCacheSize(0, cache);
165163

166164
cache.Set("key", "value", entryOptions);
167165

168166
Assert.Equal("value", cache.Get("key"));
169-
Assert.Equal(6, cache.Size);
167+
AssertCacheSize(6, cache);
170168

171169
cache.Set("key2", "value2", new MemoryCacheEntryOptions { Size = 5 });
172170

@@ -175,43 +173,43 @@ public async Task ExceedsCapacityCompacts()
175173

176174
Assert.Null(cache.Get("key"));
177175
Assert.Null(cache.Get("key2"));
178-
Assert.Equal(0, cache.Size);
176+
AssertCacheSize(0, cache);
179177
}
180178

181179
[Fact]
182180
public void AddingReplacementWithSizeIncreaseUpdates()
183181
{
184182
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });
185183

186-
Assert.Equal(0, cache.Size);
184+
AssertCacheSize(0, cache);
187185

188186
cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 2 });
189187

190188
Assert.Equal("value", cache.Get("key"));
191-
Assert.Equal(2, cache.Size);
189+
AssertCacheSize(2, cache);
192190

193191
cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 3 });
194192

195193
Assert.Equal("value1", cache.Get("key"));
196-
Assert.Equal(3, cache.Size);
194+
AssertCacheSize(3, cache);
197195
}
198196

199197
[Fact]
200198
public void AddingReplacementWithSizeDecreaseUpdates()
201199
{
202200
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });
203201

204-
Assert.Equal(0, cache.Size);
202+
AssertCacheSize(0, cache);
205203

206204
cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 2 });
207205

208206
Assert.Equal("value", cache.Get("key"));
209-
Assert.Equal(2, cache.Size);
207+
AssertCacheSize(2, cache);
210208

211209
cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 1 });
212210

213211
Assert.Equal("value1", cache.Get("key"));
214-
Assert.Equal(1, cache.Size);
212+
AssertCacheSize(1, cache);
215213
}
216214

217215
[Fact]
@@ -223,21 +221,20 @@ public void AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateAndRemoves
223221
CompactionPercentage = 0.5
224222
});
225223

226-
Assert.Equal(0, cache.Size);
224+
AssertCacheSize(0, cache);
227225

228226
cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 });
229227

230228
Assert.Equal("value", cache.Get("key"));
231-
Assert.Equal(5, cache.Size);
229+
AssertCacheSize(5, cache);
232230

233231
cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 6 });
234232

235233
Assert.Null(cache.Get("key"));
236-
Assert.Equal(0, cache.Size);
234+
AssertCacheSize(0, cache);
237235
}
238236

239237
[Fact]
240-
[ActiveIssue("https://github.com/dotnet/runtime/issues/33993")]
241238
public async Task AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateRemovesOldEntryAndTriggersCompaction()
242239
{
243240
var cache = new MemoryCache(new MemoryCacheOptions
@@ -254,20 +251,20 @@ public async Task AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateRemo
254251
State = null
255252
});
256253

257-
Assert.Equal(0, cache.Size);
254+
AssertCacheSize(0, cache);
258255

259256
cache.Set("key", "value", entryOptions);
260257

261258
Assert.Equal("value", cache.Get("key"));
262-
Assert.Equal(6, cache.Size);
259+
AssertCacheSize(6, cache);
263260

264261
cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 5 });
265262

266263
// Wait for compaction to complete
267264
Assert.True(await sem.WaitAsync(TimeSpan.FromSeconds(10)));
268265

269266
Assert.Null(cache.Get("key"));
270-
Assert.Equal(0, cache.Size);
267+
AssertCacheSize(0, cache);
271268
}
272269

273270
[Fact]
@@ -279,17 +276,18 @@ public void AddingReplacementExceedsCapacityRemovesOldEntry()
279276
CompactionPercentage = 0.5
280277
});
281278

282-
Assert.Equal(0, cache.Size);
279+
AssertCacheSize(0, cache);
283280

284281
cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 6 });
285282

286283
Assert.Equal("value", cache.Get("key"));
287-
Assert.Equal(6, cache.Size);
284+
285+
AssertCacheSize(6, cache);
288286

289287
cache.Set("key", "value1", new MemoryCacheEntryOptions { Size = 11 });
290288

291289
Assert.Null(cache.Get("key"));
292-
Assert.Equal(0, cache.Size);
290+
AssertCacheSize(0, cache); // addition was rejected due to size, and previous item with the same key removed
293291
}
294292

295293
[Fact]
@@ -299,15 +297,14 @@ public void RemovingEntryDecreasesCacheSize()
299297

300298
cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 });
301299

302-
Assert.Equal(5, cache.Size);
300+
AssertCacheSize(5, cache);
303301

304302
cache.Remove("key");
305303

306-
Assert.Equal(0, cache.Size);
304+
AssertCacheSize(0, cache);
307305
}
308306

309307
[Fact]
310-
[ActiveIssue("https://github.com/dotnet/runtime/issues/33993")]
311308
public async Task ExpiringEntryDecreasesCacheSize()
312309
{
313310
var cache = new MemoryCache(new MemoryCacheOptions
@@ -328,7 +325,7 @@ public async Task ExpiringEntryDecreasesCacheSize()
328325

329326
cache.Set("key", "value", entryOptions);
330327

331-
Assert.Equal(5, cache.Size);
328+
AssertCacheSize(5, cache);
332329

333330
// Expire entry
334331
changeToken.Fire();
@@ -339,7 +336,7 @@ public async Task ExpiringEntryDecreasesCacheSize()
339336
// Wait for compaction to complete
340337
Assert.True(await sem.WaitAsync(TimeSpan.FromSeconds(10)));
341338

342-
Assert.Equal(0, cache.Size);
339+
AssertCacheSize(0, cache);
343340
}
344341

345342
[Fact]
@@ -355,9 +352,9 @@ public void TryingToAddExpiredEntryDoesNotIncreaseCacheSize()
355352
};
356353

357354
cache.Set("key", "value", entryOptions);
358-
355+
359356
Assert.Null(cache.Get("key"));
360-
Assert.Equal(0, cache.Size);
357+
AssertCacheSize(0, cache);
361358
}
362359

363360
[Fact]
@@ -372,13 +369,12 @@ public void TryingToAddEntryWithExpiredTokenDoesNotIncreaseCacheSize()
372369
};
373370

374371
cache.Set("key", "value", entryOptions);
375-
372+
376373
Assert.Null(cache.Get("key"));
377-
Assert.Equal(0, cache.Size);
374+
AssertCacheSize(0, cache);
378375
}
379376

380377
[Fact]
381-
[ActiveIssue("https://github.com/dotnet/runtime/issues/33993")]
382378
public async Task CompactsToLessThanLowWatermarkUsingLRUWhenHighWatermarkExceeded()
383379
{
384380
var testClock = new TestClock();
@@ -449,14 +445,23 @@ public void NoCompactionWhenNoMaximumEntriesCountSpecified()
449445
public void ClearZeroesTheSize()
450446
{
451447
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });
452-
Assert.Equal(0, cache.Size);
448+
AssertCacheSize(0, cache);
453449

454450
cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 });
455-
Assert.Equal(5, cache.Size);
451+
AssertCacheSize(5, cache);
456452

457453
cache.Clear();
458-
Assert.Equal(0, cache.Size);
454+
AssertCacheSize(0, cache);
459455
Assert.Equal(0, cache.Count);
460456
}
457+
458+
internal static void AssertCacheSize(long size, MemoryCache cache)
459+
{
460+
// Size is only eventually consistent, so retry a few times
461+
RetryHelper.Execute(() =>
462+
{
463+
Assert.Equal(size, cache.Size);
464+
}, maxAttempts: 12, (iteration) => (int)Math.Pow(2, iteration)); // 2ms, 4ms.. 4096 ms. In practice, retries are rarely needed.
465+
}
461466
}
462467
}

0 commit comments

Comments
 (0)