Fix LRU clear when warm/hot items are touched (#453)

* Fix LRU clear when warm/hot items are touched

* add trim test

* special case

* cleanup

---------

Author: bitfaster

BitFaster.Caching.UnitTests/Lru/ConcurrentLruTests.cs

@@ -9,7 +9,6 @@
 using Xunit.Abstractions;
 using System.Collections.Concurrent;
 using System.Reflection;
-using System.Runtime.CompilerServices;
 
 namespace BitFaster.Caching.UnitTests.Lru
 {
@@ -875,6 +874,67 @@ public void WhenItemsExistClearRemovesAllItems()
             lru.ColdCount.Should().Be(0);
         }
 
+        // This is a special case:
+        // Cycle 1: hot => warm
+        // Cycle 2: warm => warm
+        // Cycle 3: warm => cold
+        // Cycle 4: cold => remove
+        // Cycle 5: cold => remove
+        [Fact]
+        public void WhenCacheIsSize3ItemsExistAndItemsAccessedClearRemovesAllItems()
+        {
+            lru = new ConcurrentLru<int, string>(3);
+
+            lru.AddOrUpdate(1, "1");
+            lru.AddOrUpdate(2, "1");
+
+            lru.TryGet(1, out _);
+            lru.TryGet(2, out _);
+
+            lru.Clear();
+
+            lru.Count.Should().Be(0);
+        }
+
+        [Theory]
+        [InlineData(1)]
+        [InlineData(2)]
+        [InlineData(3)]
+        [InlineData(4)]
+        [InlineData(5)]
+        [InlineData(6)]
+        [InlineData(7)]
+        [InlineData(8)]
+        [InlineData(9)]
+        [InlineData(10)]
+        public void WhenItemsExistAndItemsAccessedClearRemovesAllItems(int itemCount)
+        {
+            // By default capacity is 9. Test all possible states of touched items
+            // in the cache.
+
+            for (int i = 0; i < itemCount; i++) 
+            {
+                lru.AddOrUpdate(i, "1");
+            }
+
+            // touch n items
+            for (int i = 0; i < itemCount; i++)
+            {
+                lru.TryGet(i, out _);
+            }
+
+            lru.Clear();
+
+            this.testOutputHelper.WriteLine("LRU " + string.Join(" ", lru.Keys));
+
+            lru.Count.Should().Be(0);
+
+            // verify queues are purged
+            lru.HotCount.Should().Be(0);
+            lru.WarmCount.Should().Be(0);
+            lru.ColdCount.Should().Be(0);
+        }
+
         [Fact]
         public void WhenWarmThenClearedIsWarmIsReset()
         {
@@ -1079,6 +1139,45 @@ public void WhenColdItemsAreTouchedTrimRemovesExpectedItemCount(int trimCount, i
             this.testOutputHelper.WriteLine("exp " + string.Join(" ", expected));
 
             lru.Keys.Should().BeEquivalentTo(expected);
+        }
+
+        [Theory]
+        [InlineData(1)]
+        [InlineData(2)]
+        [InlineData(3)]
+        [InlineData(4)]
+        [InlineData(5)]
+        [InlineData(6)]
+        [InlineData(7)]
+        [InlineData(8)]
+        [InlineData(9)]
+        [InlineData(10)]
+        public void WhenItemsExistAndItemsAccessedTrimRemovesAllItems(int itemCount)
+        {
+            // By default capacity is 9. Test all possible states of touched items
+            // in the cache.
+
+            for (int i = 0; i < itemCount; i++)
+            {
+                lru.AddOrUpdate(i, "1");
+            }
+
+            // touch n items
+            for (int i = 0; i < itemCount; i++)
+            {
+                lru.TryGet(i, out _);
+            }
+
+            lru.Trim(Math.Min(itemCount, lru.Capacity));
+
+            this.testOutputHelper.WriteLine("LRU " + string.Join(" ", lru.Keys));
+
+            lru.Count.Should().Be(0);
+
+            // verify queues are purged
+            lru.HotCount.Should().Be(0);
+            lru.WarmCount.Should().Be(0);
+            lru.ColdCount.Should().Be(0);
         }
 
         [Fact]

BitFaster.Caching/Lru/ConcurrentLruCore.cs

@@ -422,15 +422,7 @@ public void AddOrUpdate(K key, V value)
         ///<inheritdoc/>
         public void Clear()
         {
-            int count = this.Count;
-
-            for (int i = 0; i < count; i++)
-            {
-                CycleHotUnchecked(ItemRemovedReason.Cleared);
-                CycleWarmUnchecked(ItemRemovedReason.Cleared);
-                TryRemoveCold(ItemRemovedReason.Cleared);
-            }
-
+            this.TrimLiveItems(itemsRemoved: 0, this.Count, ItemRemovedReason.Cleared);
             Volatile.Write(ref this.isWarm, false);
         }
 
@@ -458,7 +450,7 @@ public void Trim(int itemCount)
             // first scan each queue for discardable items and remove them immediately. Note this can remove > itemCount items.
             int itemsRemoved = this.itemPolicy.CanDiscard() ? TrimAllDiscardedItems() : 0;
 
-            TrimLiveItems(itemsRemoved, itemCount);
+            TrimLiveItems(itemsRemoved, itemCount, ItemRemovedReason.Trimmed);
         }
 
         private void TrimExpired()
@@ -507,42 +499,44 @@ void RemoveDiscardableItems(ConcurrentQueue<I> q, ref int queueCounter)
             return itemsRemoved;
         }
 
-        private void TrimLiveItems(int itemsRemoved, int itemCount)
+        private void TrimLiveItems(int itemsRemoved, int itemCount, ItemRemovedReason reason)
         {
+            // When items are touched, they are moved to warm by cycling. Therefore, to guarantee 
+            // that we can remove itemCount items, we must cycle (2 * capacity.Warm) + capacity.Hot times.
             // If clear is called during trimming, it would be possible to get stuck in an infinite
-            // loop here. Instead quit after n consecutive failed attempts to move warm/hot to cold.
+            // loop here. The warm + hot limit also guards against this case.
             int trimWarmAttempts = 0;
-            int maxAttempts = this.capacity.Cold + 1;
+            int maxWarmHotAttempts = (this.capacity.Warm * 2) + this.capacity.Hot;
 
-            while (itemsRemoved < itemCount && trimWarmAttempts < maxAttempts)
+            while (itemsRemoved < itemCount && trimWarmAttempts < maxWarmHotAttempts)
             {
                 if (Volatile.Read(ref this.counter.cold) > 0)
                 {
-                    if (TryRemoveCold(ItemRemovedReason.Trimmed) == (ItemDestination.Remove, 0))
+                    if (TryRemoveCold(reason) == (ItemDestination.Remove, 0))
                     {
                         itemsRemoved++;
                         trimWarmAttempts = 0;
                     }
 
-                    TrimWarmOrHot();
+                    TrimWarmOrHot(reason);
                 }
                 else
                 {
-                    TrimWarmOrHot();
+                    TrimWarmOrHot(reason);
                     trimWarmAttempts++;
                 }
             }
         }
 
-        private void TrimWarmOrHot()
+        private void TrimWarmOrHot(ItemRemovedReason reason)
         {
             if (Volatile.Read(ref this.counter.warm) > 0)
             {
-                CycleWarmUnchecked(ItemRemovedReason.Trimmed);
+                CycleWarmUnchecked(reason);
             }
             else if (Volatile.Read(ref this.counter.hot) > 0)
             {
-                CycleHotUnchecked(ItemRemovedReason.Trimmed);
+                CycleHotUnchecked(reason);
             }
         }