diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 35121fc86ebc3..d9fa37e8cf834 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -395,9 +395,10 @@ public void Compact(double percentage) private void Compact(long removalSizeTarget, Func computeEntrySize) { var entriesToRemove = new List(); - var lowPriEntries = new List(); - var normalPriEntries = new List(); - var highPriEntries = new List(); + // cache LastAccessed outside of the CacheEntry so it is stable during compaction + var lowPriEntries = new List(); + var normalPriEntries = new List(); + var highPriEntries = new List(); long removedSize = 0; // Sort items by expired & priority status @@ -415,13 +416,13 @@ private void Compact(long removalSizeTarget, Func computeEntry switch (entry.Priority) { case CacheItemPriority.Low: - lowPriEntries.Add(entry); + lowPriEntries.Add(new CompactPriorityEntry(entry, entry.LastAccessed)); break; case CacheItemPriority.Normal: - normalPriEntries.Add(entry); + normalPriEntries.Add(new CompactPriorityEntry(entry, entry.LastAccessed)); break; case CacheItemPriority.High: - highPriEntries.Add(entry); + highPriEntries.Add(new CompactPriorityEntry(entry, entry.LastAccessed)); break; case CacheItemPriority.NeverRemove: break; @@ -445,7 +446,7 @@ private void Compact(long removalSizeTarget, Func computeEntry // ?. Items with the soonest absolute expiration. // ?. Items with the soonest sliding expiration. // ?. Larger objects - estimated by object graph size, inaccurate. - static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, Func computeEntrySize, List entriesToRemove, List priorityEntries) + static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, Func computeEntrySize, List entriesToRemove, List priorityEntries) { // Do we meet our quota by just removing expired entries? if (removalSizeTarget <= removedSize) @@ -458,9 +459,10 @@ static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, F // TODO: Refine policy // LRU - priorityEntries.Sort((e1, e2) => e1.LastAccessed.CompareTo(e2.LastAccessed)); - foreach (CacheEntry entry in priorityEntries) + priorityEntries.Sort(static (e1, e2) => e1.LastAccessed.CompareTo(e2.LastAccessed)); + foreach (CompactPriorityEntry priorityEntry in priorityEntries) { + CacheEntry entry = priorityEntry.Entry; entry.SetExpired(EvictionReason.Capacity); entriesToRemove.Add(entry); removedSize += computeEntrySize(entry); @@ -473,6 +475,20 @@ static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, F } } + // use a struct instead of a ValueTuple to avoid adding a new dependency + // on System.ValueTuple on .NET Framework in a servicing release + private readonly struct CompactPriorityEntry + { + public readonly CacheEntry Entry; + public readonly DateTimeOffset LastAccessed; + + public CompactPriorityEntry(CacheEntry entry, DateTimeOffset lastAccessed) + { + Entry = entry; + LastAccessed = lastAccessed; + } + } + public void Dispose() { Dispose(true); diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj b/src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj index 495c645ba993a..88bf4c5a54b26 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj @@ -4,6 +4,8 @@ netstandard2.0;net461 true In-memory cache implementation of Microsoft.Extensions.Caching.Memory.IMemoryCache. + true + 1 diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs index 3d9c2625b19ed..10b9cce7c4d45 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs @@ -2,6 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Threading; +using System.Threading.Tasks; using Microsoft.Extensions.Internal; using Xunit; @@ -83,4 +85,60 @@ public void CompactPrioritizesLRU() Assert.Equal("value4", cache.Get("key4")); } } + + [CollectionDefinition(nameof(DisableParallelization), DisableParallelization = true)] + public class DisableParallelization { } + + [Collection(nameof(DisableParallelization))] + public class CompactTestsDisableParallelization + { + /// + /// Tests a race condition in Compact where CacheEntry.LastAccessed is getting updated + /// by a different thread than what is doing the Compact, leading to sorting failing. + /// + /// See https://github.com/dotnet/runtime/issues/61032. + /// + [Fact] + public void CompactLastAccessedRaceCondition() + { + const int numEntries = 100; + MemoryCache cache = new MemoryCache(new MemoryCacheOptions()); + Random random = new Random(); + + void FillCache() + { + for (int i = 0; i < numEntries; i++) + { + cache.Set($"key{i}", $"value{i}"); + } + } + + // start a few tasks to access entries in the background + Task[] backgroundAccessTasks = new Task[Environment.ProcessorCount]; + bool done = false; + + for (int i = 0; i < backgroundAccessTasks.Length; i++) + { + backgroundAccessTasks[i] = Task.Run(async () => + { + while (!done) + { + cache.TryGetValue($"key{random.Next(numEntries)}", out _); + await Task.Yield(); + } + }); + } + + for (int i = 0; i < 1000; i++) + { + FillCache(); + + cache.Compact(1); + } + + done = true; + + Task.WaitAll(backgroundAccessTasks); + } + } }