Skip to content

Fix MemoryCache test failures due to race #72821

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.

Sign up for GitHub

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

Merged
danmoseley merged 7 commits into from
Jul 26, 2022
Merged

Fix MemoryCache test failures due to race #72821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fb2ee06
Improve comments
danmoseley Jul 25, 2022
25cb29b
Re-enable tests disabled for 33993
danmoseley Jul 25, 2022
7396280
Add comment
danmoseley Jul 25, 2022
dbf6069
Fix tests using retries
danmoseley Jul 25, 2022
ee4f015
Comment
danmoseley Jul 25, 2022
49eb05a
comment
danmoseley Jul 26, 2022
874004d
comment
danmoseley Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/Microsoft.Extensions.Caching.Abstractions/src/ICacheEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ namespace Microsoft.Extensions.Caching.Memory
{
/// <summary>
/// Represents an entry in the <see cref="IMemoryCache"/> implementation.
/// When Disposed, is committed to the cache.
/// </summary>
public interface ICacheEntry : IDisposable
{
Expand Down
27 changes: 26 additions & 1 deletion src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,12 @@ public MemoryCache(IOptions<MemoryCacheOptions> optionsAccessor, ILoggerFactory
/// </summary>
public int Count => _coherentState.Count;

// internal for testing
/// <summary>
/// Internal accessor for Size for testing only.
///
/// Note that this is only eventually consistent with the contents of the collection.
/// See comment on <see cref="CoherentState"/>.
/// </summary>
internal long Size => _coherentState.Size;

internal bool TrackLinkedCacheEntries { get; }
Expand Down Expand Up @@ -421,6 +426,10 @@ private void ScanForExpiredItems()
}
}

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

/// <summary>
/// Wrapper for the memory cache entries collection.
///
/// Entries may have various sizes. If a size limit has been set, the cache keeps track of the aggregate of all the entries' sizes
/// in order to trigger compaction when the size limit is exceeded.
///
/// For performance reasons, the size is not updated atomically with the collection, but is only made eventually consistent.
///
/// When the memory cache is cleared, it replaces the backing collection entirely. This may occur in parallel with operations
/// like add, set, remove, and compact which may modify the collection and thus its overall size.
///
/// To keep the overall size eventually consistent, therefore, the collection and the overall size are wrapped in this CoherentState
/// object. Individual operations take a local reference to this wrapper object while they work, and make size updates to this object.
/// Clearing the cache simply replaces the object, so that any still in progress updates do not affect the overall size value for
/// the new backing collection.
/// </summary>
private sealed class CoherentState
{
internal ConcurrentDictionary<object, CacheEntry> _entries = new ConcurrentDictionary<object, CacheEntry>();
Expand Down
93 changes: 49 additions & 44 deletions src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,45 +72,44 @@ public void AddingEntryIncreasesCacheSizeWhenEnforcingSizeLimit()
{
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal(5, cache.Size);
AssertCacheSize(5, cache);
}

[Fact]
public void AddingEntryDoesNotIncreasesCacheSizeWhenNotEnforcingSizeLimit()
{
var cache = new MemoryCache(new MemoryCacheOptions());

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
}

[Fact]
public void DoNotAddEntryIfItExceedsCapacity()
{
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal("value", cache.Get("key"));
Assert.Equal(4, cache.Size);
AssertCacheSize(4, cache);

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

Assert.Null(cache.Get("key2"));
Assert.Equal(4, cache.Size);
AssertCacheSize(4, cache);
}

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

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal("value", cache.Get("key"));
Assert.Equal(long.MaxValue, cache.Size);
AssertCacheSize(long.MaxValue, cache);

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

// Compaction removes old item
Assert.Null(cache.Get("key"));
Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/33993")]
public async Task ExceedsCapacityCompacts()
{
var cache = new MemoryCache(new MemoryCacheOptions
Expand All @@ -161,12 +159,12 @@ public async Task ExceedsCapacityCompacts()
State = null
});

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal("value", cache.Get("key"));
Assert.Equal(6, cache.Size);
AssertCacheSize(6, cache);

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

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

Assert.Null(cache.Get("key"));
Assert.Null(cache.Get("key2"));
Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
}

[Fact]
public void AddingReplacementWithSizeIncreaseUpdates()
{
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal("value", cache.Get("key"));
Assert.Equal(2, cache.Size);
AssertCacheSize(2, cache);

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

Assert.Equal("value1", cache.Get("key"));
Assert.Equal(3, cache.Size);
AssertCacheSize(3, cache);
}

[Fact]
public void AddingReplacementWithSizeDecreaseUpdates()
{
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal("value", cache.Get("key"));
Assert.Equal(2, cache.Size);
AssertCacheSize(2, cache);

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

Assert.Equal("value1", cache.Get("key"));
Assert.Equal(1, cache.Size);
AssertCacheSize(1, cache);
}

[Fact]
Expand All @@ -223,21 +221,20 @@ public void AddingReplacementWhenTotalSizeExceedsCapacityDoesNotUpdateAndRemoves
CompactionPercentage = 0.5
});

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal("value", cache.Get("key"));
Assert.Equal(5, cache.Size);
AssertCacheSize(5, cache);

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

Assert.Null(cache.Get("key"));
Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
}

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

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal("value", cache.Get("key"));
Assert.Equal(6, cache.Size);
AssertCacheSize(6, cache);

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

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

Assert.Null(cache.Get("key"));
Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
}

[Fact]
Expand All @@ -279,17 +276,18 @@ public void AddingReplacementExceedsCapacityRemovesOldEntry()
CompactionPercentage = 0.5
});

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

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

Assert.Equal("value", cache.Get("key"));
Assert.Equal(6, cache.Size);

AssertCacheSize(6, cache);

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

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

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

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

Assert.Equal(5, cache.Size);
AssertCacheSize(5, cache);

cache.Remove("key");

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
}

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

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

Assert.Equal(5, cache.Size);
AssertCacheSize(5, cache);

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

Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
}

[Fact]
Expand All @@ -355,9 +352,9 @@ public void TryingToAddExpiredEntryDoesNotIncreaseCacheSize()
};

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

Assert.Null(cache.Get("key"));
Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
}

[Fact]
Expand All @@ -372,13 +369,12 @@ public void TryingToAddEntryWithExpiredTokenDoesNotIncreaseCacheSize()
};

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

Assert.Null(cache.Get("key"));
Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/33993")]
public async Task CompactsToLessThanLowWatermarkUsingLRUWhenHighWatermarkExceeded()
{
var testClock = new TestClock();
Expand Down Expand Up @@ -449,14 +445,23 @@ public void NoCompactionWhenNoMaximumEntriesCountSpecified()
public void ClearZeroesTheSize()
{
var cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 10 });
Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);

cache.Set("key", "value", new MemoryCacheEntryOptions { Size = 5 });
Assert.Equal(5, cache.Size);
AssertCacheSize(5, cache);

cache.Clear();
Assert.Equal(0, cache.Size);
AssertCacheSize(0, cache);
Assert.Equal(0, cache.Count);
}

internal static void AssertCacheSize(long size, MemoryCache cache)
{
// Size is only eventually consistent, so retry a few times
RetryHelper.Execute(() =>
{
Assert.Equal(size, cache.Size);
}, maxAttempts: 12, (iteration) => (int)Math.Pow(2, iteration)); // 2ms, 4ms.. 4096 ms. In practice, retries are rarely needed.
}
}
}
Loading