[release/6.0] Cache LastAccessed during MemoryCache compaction

src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs

@@ -395,9 +395,10 @@ public void Compact(double percentage)
         private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntrySize)
         {
             var entriesToRemove = new List<CacheEntry>();
-            var lowPriEntries = new List<CacheEntry>();
-            var normalPriEntries = new List<CacheEntry>();
-            var highPriEntries = new List<CacheEntry>();
+            // cache LastAccessed outside of the CacheEntry so it is stable during compaction
+            var lowPriEntries = new List<CompactPriorityEntry>();
+            var normalPriEntries = new List<CompactPriorityEntry>();
+            var highPriEntries = new List<CompactPriorityEntry>();
             long removedSize = 0;
 
             // Sort items by expired & priority status
@@ -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));
                             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<CacheEntry, long> 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<CacheEntry, long> computeEntrySize, List<CacheEntry> entriesToRemove, List<CacheEntry> priorityEntries)
+            static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, Func<CacheEntry, long> computeEntrySize, List<CacheEntry> entriesToRemove, List<CompactPriorityEntry> 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);

src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj

@@ -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>
   </PropertyGroup>
 
   <ItemGroup>

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
+    {
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [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);
+        }
+    }
 }