From d1a1e97609d42f7b84a27d22ca445f1a94627c97 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Wed, 1 Dec 2021 18:22:22 -0600 Subject: [PATCH 1/2] 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 35121fc86ebc34..f3d29daf106e77 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<(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 @@ -415,13 +416,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; @@ -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<(CacheEntry Entry, DateTimeOffset LastAccessed)> priorityEntries) { // Do we meet our quota by just removing expired entries? if (removalSizeTarget <= removedSize) @@ -458,8 +459,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 495c645ba993ad..b630f4d43ea54d 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 @@ -14,4 +14,8 @@ + + + + diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs index 3d9c2625b19ed9..5568b9213360dd 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 7046da24da6d6d..c631b1c230efad 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 + + + + From dfead377aade7f2c36fe07f58f62ecaeef3b71b2 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Thu, 2 Dec 2021 11:14:38 -0600 Subject: [PATCH 2/2] 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. --- .../src/MemoryCache.cs | 31 ++++++++++++++----- ...Microsoft.Extensions.Caching.Memory.csproj | 6 ++-- .../tests/CompactTests.cs | 3 ++ ...oft.Extensions.Caching.Memory.Tests.csproj | 5 --- 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index f3d29daf106e77..d9fa37e8cf8343 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -396,9 +396,9 @@ private void Compact(long removalSizeTarget, Func computeEntry { var entriesToRemove = 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)>(); + var lowPriEntries = new List(); + var normalPriEntries = new List(); + var highPriEntries = new List(); long removedSize = 0; // Sort items by expired & priority status @@ -416,13 +416,13 @@ private void Compact(long removalSizeTarget, Func computeEntry switch (entry.Priority) { case CacheItemPriority.Low: - lowPriEntries.Add((entry, entry.LastAccessed)); + lowPriEntries.Add(new CompactPriorityEntry(entry, entry.LastAccessed)); break; case CacheItemPriority.Normal: - normalPriEntries.Add((entry, entry.LastAccessed)); + normalPriEntries.Add(new CompactPriorityEntry(entry, entry.LastAccessed)); break; case CacheItemPriority.High: - highPriEntries.Add((entry, entry.LastAccessed)); + highPriEntries.Add(new CompactPriorityEntry(entry, entry.LastAccessed)); break; case CacheItemPriority.NeverRemove: break; @@ -446,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<(CacheEntry Entry, DateTimeOffset LastAccessed)> 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) @@ -460,8 +460,9 @@ static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, F // LRU priorityEntries.Sort(static (e1, e2) => e1.LastAccessed.CompareTo(e2.LastAccessed)); - foreach ((CacheEntry entry, _) in priorityEntries) + foreach (CompactPriorityEntry priorityEntry in priorityEntries) { + CacheEntry entry = priorityEntry.Entry; entry.SetExpired(EvictionReason.Capacity); entriesToRemove.Add(entry); removedSize += computeEntrySize(entry); @@ -474,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 b630f4d43ea54d..88bf4c5a54b262 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 @@ -14,8 +16,4 @@ - - - - diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs index 5568b9213360dd..10b9cce7c4d459 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/CompactTests.cs @@ -86,6 +86,9 @@ public void CompactPrioritizesLRU() } } + [CollectionDefinition(nameof(DisableParallelization), DisableParallelization = true)] + public class DisableParallelization { } + [Collection(nameof(DisableParallelization))] public class CompactTestsDisableParallelization { 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 c631b1c230efad..7046da24da6d6d 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,11 +5,6 @@ true - - - -