From b1fcbb68ecc57eec7b579a29d3982d5299f95673 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Wed, 1 Dec 2021 18:22:22 -0600 Subject: [PATCH] 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 --- .../src/MemoryCache.cs | 19 ++++--- ...Microsoft.Extensions.Caching.Memory.csproj | 4 ++ .../tests/CompactTests.cs | 55 +++++++++++++++++++ ...oft.Extensions.Caching.Memory.Tests.csproj | 5 ++ 4 files changed, 74 insertions(+), 9 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index b0d6c71eb6008..b5775612b2c1a 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -391,9 +391,10 @@ public void Compact(double percentage) private void Compact(long removalSizeTarget, Func computeEntrySize, CoherentState coherentState) { 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<(CacheEntry entry, DateTimeOffset lastAccessed)>(); + var normalPriEntries = new List<(CacheEntry entry, DateTimeOffset lastAccessed)>(); + var highPriEntries = new List<(CacheEntry entry, DateTimeOffset lastAccessed)>(); long removedSize = 0; // Sort items by expired & priority status @@ -411,13 +412,13 @@ private void Compact(long removalSizeTarget, Func computeEntry switch (entry.Priority) { case CacheItemPriority.Low: - lowPriEntries.Add(entry); + lowPriEntries.Add((entry, entry.LastAccessed)); break; case CacheItemPriority.Normal: - normalPriEntries.Add(entry); + normalPriEntries.Add((entry, entry.LastAccessed)); break; case CacheItemPriority.High: - highPriEntries.Add(entry); + highPriEntries.Add((entry, entry.LastAccessed)); break; case CacheItemPriority.NeverRemove: break; @@ -441,7 +442,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<(CacheEntry Entry, DateTimeOffset LastAccessed)> priorityEntries) { // Do we meet our quota by just removing expired entries? if (removalSizeTarget <= removedSize) @@ -454,8 +455,8 @@ 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 ((CacheEntry entry, _) in priorityEntries) { entry.SetExpired(EvictionReason.Capacity); entriesToRemove.Add(entry); 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 0a8f2ee16a522..a75f35e5dc64c 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 @@ -16,4 +16,8 @@ + + + + diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs index 3d9c2625b19ed..5568b9213360d 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,57 @@ public void CompactPrioritizesLRU() Assert.Equal("value4", cache.Get("key4")); } } + + [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); + } + } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj index 5704f98966885..9773db13ad771 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj @@ -5,6 +5,11 @@ true + + + +