Cache LastAccessed during MemoryCache compaction #61187
Conversation
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
|
Tagging subscribers to this area: @eerhardt, @maryamariyan, @michaelgsharp Issue DetailsDuring 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 #61032
|
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'"> | ||
| <PackageReference Include="System.ValueTuple" Version="$(SystemValueTupleVersion)" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
Backporting a fix that has new dependencies seems problematic.
There was a problem hiding this comment.
Should I create a new private struct for this data instead of using a ValueTuple?
There was a problem hiding this comment.
@ericstj @davidfowl - any thoughts here? If we decide to backport this to 6.0.x, would this new dependency on the System.ValueTuple package be a problem? Nothing in MS.Ext.Caching.Memory brings in this package today on netfx.
I can easily switch to not use value tuples, but it seems like a pain going forward.
One option could be use value tuples in 7.0 going forward, and just use a custom struct in the 6.0 version.
There was a problem hiding this comment.
I would just stick to a small internal struct for now. It's much less intrusive and doesn't require an ifdef. We should then decide when we rip the netstandard 2.0 cord. (.NET 7 or 8?)
There was a problem hiding this comment.
doesn't require an ifdef
The plan I'm proposing doesn't require an ifdef (beyond this PackageReference). The idea is:
For the 7.0.0+ versions of this package (in main) we use ValueTuple like I am here. Then it is available in the future if we want to use it in other places. If servicing 6.0 wasn't a consideration, this is the plan I would take without a 2nd thought.
For the 6.0.1 version of this package (in release/6.0) change the ValueTuple uses in this PR to a small struct and remove the dependency on System.ValueTuple. That way we don't add the new dependency in a service release.
We should then decide when we rip the netstandard 2.0 cord. (.NET 7 or 8?)
The plan above fits great with this. If/when we rip the netstandard2.0 cord, this PackageReference goes away and the C# code doesn't change at all, since ValueTuple is available inbox on NETCOREAPP.
There was a problem hiding this comment.
The ValueTuple dependency can be a real PITA for users who are using a very old VS that generates invalid binding redirects for .NET Framework projects: dotnet/BenchmarkDotNet#1687 (comment)
I doubt that they use latest MemoryCache version and I think that we should just stop targeting .NET Standard 2.0. But we need to verify that with some data.
+1 |
I plan on taking this approach unless I hear objection. I plan on merging this for 7.0 once CI is green. When I backport to 6.0, I will make the appropriate changes to not add an additional dependency in the |
- Pulling MemoryCache fixes from dotnet/runtime#57631 and dotnet/runtime#61187
- Pulling MemoryCache fixes from dotnet/runtime#57631 and dotnet/runtime#61187
| <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'"> | ||
| <PackageReference Include="System.ValueTuple" Version="$(SystemValueTupleVersion)" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
The ValueTuple dependency can be a real PITA for users who are using a very old VS that generates invalid binding redirects for .NET Framework projects: dotnet/BenchmarkDotNet#1687 (comment)
I doubt that they use latest MemoryCache version and I think that we should just stop targeting .NET Standard 2.0. But we need to verify that with some data.
We use it elsewhere: I'd guess the odds that users using very old VS on netfx using the latest version of this package, but not any of those is pretty low. |
* 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
* Cache LastAccessed during MemoryCache compaction (#61187) * 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 #61032 * Update fix for 6.0.x servicing. 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.
ghost
locked as resolved and limited conversation to collaborators
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 #61032