-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
[release/6.0] Cache LastAccessed during MemoryCache compaction #62286
Conversation
* Cache LastAccessed during MemoryCache compaction During cache compaction, we are sorting entries based on their LastAccessed time. However, since the cache entries can still be used concurrently on other threads, the LastAccessed time may be updated in the middle of sorting the entries. This leads to exceptions in a background thread, crashing the process. The fix is to cache the LastAccessed time outside of the entry when we are adding it to the list. This will ensure the time is stable during the compaction process. Fix dotnet#61032
1. Remove the dependency on ValueTuple and use a custom struct instead. 2. Add servicing package changes. 3. In the tests, move the DisableParallelization collection declaration in the test project, since it is only in "Common" in main.
Tagging subscribers to this area: @dotnet/area-extensions-caching Issue DetailsBackport of #61187 to release/6.0 Customer ImpactThere is a race condition in MemoryCache compaction that can cause apps to crash (since the compaction happens on a background thread, and an unhandled exception can be thrown). This can seemingly crash their process randomly. TestingAdded a new multi-threaded test that tests the scenario. RiskLow Risk. The change is to snapshot the Note for reviewersI have 2 commits. The first commit is what was merged into Fix #61032
|
@@ -415,13 +416,13 @@ private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntry | |||
switch (entry.Priority) | |||
{ | |||
case CacheItemPriority.Low: | |||
lowPriEntries.Add(entry); | |||
lowPriEntries.Add(new CompactPriorityEntry(entry, entry.LastAccessed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there will not be any boxing operation that causes additional allocations. right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. We are using a List<CompactPriorityEntry>
, so no boxing will occur. The small structs will just be copied in and out of the list. This is the same with the 7.0+ implementation, but there we are using ValueTuple instead of a custom struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @eerhardt !
@@ -4,6 +4,8 @@ | |||
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks> | |||
<EnableDefaultItems>true</EnableDefaultItems> | |||
<PackageDescription>In-memory cache implementation of Microsoft.Extensions.Caching.Memory.IMemoryCache.</PackageDescription> | |||
<GeneratePackageOnBuild>true</GeneratePackageOnBuild> | |||
<ServicingVersion>1</ServicingVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@safern - this is all I need to do to service this package, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packaging updates look good to me.
Backport of #61187 to release/6.0
Customer Impact
There is a race condition in MemoryCache compaction that can cause apps to crash (since the compaction happens on a background thread, and an unhandled exception can be thrown). This can seemingly crash their process randomly.
Testing
Added a new multi-threaded test that tests the scenario.
Risk
Low Risk. The change is to snapshot the
LastAccessed
value, so even if it is updated during compaction, it won't cause an issue.Note for reviewers
I have 2 commits. The first commit is what was merged into
main
with #61187. The second commit are the changes necessary for servicing.Fix #61032