Fix MemoryCache concurrency tests - #131728
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Updates Microsoft.Extensions.Caching.Memory capacity/concurrency tests to be more deterministic and less prone to hangs/timeouts by using bounded worker lifetimes, dedicated worker threads (to avoid thread-pool starvation), and “eventually consistent” retry assertions where the cache is known to settle asynchronously.
Changes:
- Reworked several thread-safety/capacity tests to use fixed iteration counts + barriers and a shared worker wait helper with a timeout (instead of cancellation/time-based loops).
- Added
CapacityTests.AssertEventuallyand refactoredAssertCacheSizeto use it, improving reliability whenMemoryCache.Sizeis only eventually consistent. - Removed
ActiveIssue(72890)skips from affected tests after making them bounded/retry-based.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs | Makes concurrency/capacity tests bounded and less likely to starve the thread pool; adds shared worker helpers and uses CapacityTests.AssertEventually for settling assertions. |
| src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs | Introduces AssertEventually and updates AssertCacheSize to rely on it, clarifying and codifying retry semantics for eventually-consistent cache state. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs:853
- Same as above: XML doc comments in tests add noise and don't feed public docs. Prefer a normal // comment here.
/// <summary>
/// Waits for the workers, failing rather than hanging if they do not finish. Those tests exist to
/// catch deadlocks and livelocks in <see cref="MemoryCache"/>, so an unbounded wait would turn the
/// very bug they hunt into an unattributable CI job timeout instead of a test failure.
/// </summary>
src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs:847
- XML documentation comments are discouraged in test code (they're intended for public API docs and increase noise in tests). Consider converting this
block to a regular // comment (or removing it) to match typical test style.
This issue also appears on line 849 of the same file.
/// <summary>
/// Runs <paramref name="work"/> on a dedicated thread rather than a thread pool thread. The
/// concurrency tests above hammer the cache for their whole run without ever yielding, so leaving
/// them on the pool would delay the cache's own background work (overcapacity compaction, expired
/// item scans) and the sibling test collections xunit runs in parallel. Note this frees pool
src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs:503
- XML doc comments in test helper code are discouraged; prefer a normal // comment to avoid generating doc-comment noise in tests.
/// <summary>
/// Retries <paramref name="assert"/> until the cache state it inspects settles. Every value the
/// assertion depends on must be read inside the callback.
/// </summary>
internal static void AssertEventually(Action assert, [CallerMemberName] string? testName = null) =>
Re-land of #129897, which was reverted by #130152 after failing in CI (#130139).
Fixes #72890
Motivation
Three
MemoryCacheconcurrency tests inMemoryCacheSetAndRemoveTestshave been disabled since 2022 because they were racy:GetAndSet_AreThreadSafe_AndUpdatesNeverLeavesNullValuesOvercapacityPurge_AreThreadSafeAddAndReplaceEntries_AreThreadSafeWhat was wrong with #129897
CapacityTests.AssertCacheSize(long size, MemoryCache cache)retriesAssert.Equal(size, cache.Size), but re-reads onlycache.Size. #129897 called it as:so
cache.Countwas evaluated once, eagerly, at the call site and then frozen for the whole retry loop.Overcapacity compaction is asynchronous:
SetEntry→TriggerOvercapacityCompaction→ThreadPool.UnsafeQueueUserWorkItem→OvercapacityCompaction. A compaction queued by the last write can land after the snapshot is taken, halving the cache (SizeLimit = 10,CompactionPercentage = 0.5). The frozen expected value can then never be matched, and all 12 retries fail.The CI stack trace confirms the location exactly:
MemoryCacheSetAndRemoveTests.cs(654,0)in the #129897 revision is thatAssertCacheSize(cache.Count, cache)line.The pre-#129897 version avoided this only by accident. Writers were cancelled at 5 seconds and the test waited until 7 seconds, leaving a 2-second quiescent window in which any pending compaction finished and no new one could be triggered. #129897 correctly removed the arbitrary delays, but nothing replaced that quiescence.
I proved the diagnosis with a standalone harness modelling the assertion's snapshot behaviour:
Countsnapshotted once (as in #129897)Countre-read on every retry (this PR)The fix
CapacityTestsgains a sharedAssertEventually(Action)helper that re-reads every value inside the callback, andAssertCacheSizenow delegates to it and carries a comment warning that its expected size must be a constant, never read from the cache.