Enhancements to SlidingCache<> for Improved Thread Safety and Performance (#770)

* #769 - Enhancements to SlidingCache<> for Improved Thread Safety and Performance

* #769 - Rename Config ReturnExpiredItems; Refactor Tests

* #769 - SlidingCache Test Code-Comment Cleanup

---------

Co-authored-by: Travis Whidden <[email protected]>

Author: TWhidden

src/System.Linq.Dynamic.Core/Util/Cache/CacheConfig.cs

@@ -24,4 +24,13 @@ public class CacheConfig
     /// By default, cleanup occurs every 10 minutes.
     /// </summary>
     public TimeSpan CleanupFrequency { get; set; } = SlidingCacheConstants.DefaultCleanupFrequency;
+
+    /// <summary>
+    /// Enables returning expired cache items in scenarios where cleanup, running on a separate thread, 
+    /// has not yet removed them. This allows for the retrieval of an expired object without needing to 
+    /// clear and recreate it if a request is made concurrently with cleanup. Particularly useful 
+    /// when cached items are deterministic, ensuring consistent results even from expired entries.
+    /// Default true;
+    /// </summary>
+    public bool ReturnExpiredItems { get; set; } = true;
 }

src/System.Linq.Dynamic.Core/Util/Cache/SlidingCache.cs

@@ -1,6 +1,7 @@
 using System.Collections.Concurrent;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq.Dynamic.Core.Validation;
+using System.Threading;
 
 namespace System.Linq.Dynamic.Core.Util.Cache;
 
@@ -11,7 +12,9 @@ internal class SlidingCache<TKey, TValue> where TKey : notnull where TValue : no
     private readonly IDateTimeUtils _dateTimeProvider;
     private readonly Action _deleteExpiredCachedItemsDelegate;
     private readonly long? _minCacheItemsBeforeCleanup;
+    private readonly bool _returnExpiredItems;
     private DateTime _lastCleanupTime;
+    private int _cleanupLocked = 0;
 
     /// <summary>
     ///     Sliding Thread Safe Cache
@@ -26,15 +29,19 @@ internal class SlidingCache<TKey, TValue> where TKey : notnull where TValue : no
     ///     Provides the Time for the Caching object. Default will be created if not supplied. Used
     ///     for Testing classes
     /// </param>
+    /// <param name="returnExpiredItems">If a request for an item happens to be expired, but is still
+    /// in known, don't expire it and return it instead.</param>
     public SlidingCache(
         TimeSpan timeToLive,
         TimeSpan? cleanupFrequency = null,
         long? minCacheItemsBeforeCleanup = null,
-        IDateTimeUtils? dateTimeProvider = null)
+        IDateTimeUtils? dateTimeProvider = null,
+        bool returnExpiredItems = false)
     {
         _cache = new ConcurrentDictionary<TKey, CacheEntry<TValue>>();
         TimeToLive = timeToLive;
         _minCacheItemsBeforeCleanup = minCacheItemsBeforeCleanup;
+        _returnExpiredItems = returnExpiredItems;
         _cleanupFrequency = cleanupFrequency ?? SlidingCacheConstants.DefaultCleanupFrequency;
         _deleteExpiredCachedItemsDelegate = Cleanup;
         _dateTimeProvider = dateTimeProvider ?? new DateTimeUtils();
@@ -58,6 +65,7 @@ public SlidingCache(CacheConfig cacheConfig, IDateTimeUtils? dateTimeProvider =
         TimeToLive = cacheConfig.TimeToLive;
         _minCacheItemsBeforeCleanup = cacheConfig.MinItemsTrigger;
         _cleanupFrequency = cacheConfig.CleanupFrequency;
+        _returnExpiredItems = cacheConfig.ReturnExpiredItems;
         _deleteExpiredCachedItemsDelegate = Cleanup;
         _dateTimeProvider = dateTimeProvider ?? new DateTimeUtils();
         // To prevent a scan on first call, set the last Cleanup to the current Provider time
@@ -100,20 +108,29 @@ public bool TryGetValue(TKey key, [NotNullWhen(true)] out TValue? value)
     {
         Check.NotNull(key);
 
-        CleanupIfNeeded();
-
-        if (_cache.TryGetValue(key, out var valueAndExpiration))
+        try
         {
-            if (_dateTimeProvider.UtcNow <= valueAndExpiration.ExpirationTime)
+            if (_cache.TryGetValue(key, out var valueAndExpiration))
             {
-                value = valueAndExpiration.Value;
-                var newExpire = _dateTimeProvider.UtcNow.Add(TimeToLive);
-                _cache[key] = new CacheEntry<TValue>(value, newExpire);
-                return true;
+                // Permit expired returns will return the object even if was expired
+                // this will prevent the need to re-create the object for the caller
+                if (_returnExpiredItems || _dateTimeProvider.UtcNow <= valueAndExpiration.ExpirationTime)
+                {
+                    value = valueAndExpiration.Value;
+                    var newExpire = _dateTimeProvider.UtcNow.Add(TimeToLive);
+                    _cache[key] = new CacheEntry<TValue>(value, newExpire);
+                    return true;
+                }
+
+                // Remove expired item
+                _cache.TryRemove(key, out _);
             }
-
-            // Remove expired item
-            _cache.TryRemove(key, out _);
+        }
+        finally
+        {
+            // If permit expired returns are enabled,
+            // we want to ensure the cache has a chance to get the value
+            CleanupIfNeeded();
         }
 
         value = default;
@@ -131,26 +148,41 @@ public bool Remove(TKey key)
 
     private void CleanupIfNeeded()
     {
-        if (_dateTimeProvider.UtcNow - _lastCleanupTime > _cleanupFrequency
-            && (_minCacheItemsBeforeCleanup == null ||
-                _cache.Count >=
-                _minCacheItemsBeforeCleanup) // Only cleanup if we have a minimum number of items in the cache.
-           )
+        // Ensure this is only executing one at a time.
+        if (Interlocked.CompareExchange(ref _cleanupLocked, 1, 0) != 0)
+        {
+            return;
+        }
+
+        try
         {
-            // Set here, so we don't have re-entry due to large collection enumeration.
-            _lastCleanupTime = _dateTimeProvider.UtcNow;
+            if (_dateTimeProvider.UtcNow - _lastCleanupTime > _cleanupFrequency
+                && (_minCacheItemsBeforeCleanup == null ||
+                    _cache.Count >=
+                    _minCacheItemsBeforeCleanup) // Only cleanup if we have a minimum number of items in the cache.
+               )
+            {
+                // Set here, so we don't have re-entry due to large collection enumeration.
+                _lastCleanupTime = _dateTimeProvider.UtcNow;
 
-            TaskUtils.Run(_deleteExpiredCachedItemsDelegate);
+                TaskUtils.Run(_deleteExpiredCachedItemsDelegate);
+            }
+        }
+        finally
+        {
+            // Release the lock
+            _cleanupLocked = 0;
         }
     }
 
     private void Cleanup()
     {
-        foreach (var key in _cache.Keys)
+        // Enumerate the key/value - safe per docs
+        foreach (var keyValue in _cache)
         {
-            if (_dateTimeProvider.UtcNow > _cache[key].ExpirationTime)
+            if (_dateTimeProvider.UtcNow > keyValue.Value.ExpirationTime)
             {
-                _cache.TryRemove(key, out _);
+                _cache.TryRemove(keyValue.Key, out _);
             }
         }
     }

test/System.Linq.Dynamic.Core.Tests/Util/Cache/SlidingCacheTests.cs

@@ -15,6 +15,7 @@ public class SlidingCacheTests
     public void SlidingCache_CacheOperations()
     {
         var dateTimeUtilsMock = new Mock<IDateTimeUtils>();
+        // Configure Mock with SetupGet since SlidingCache can be non-deterministic; don't use SetupSequence
         dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(UtcNow);
 
         // Arrange
@@ -46,27 +47,51 @@ public void SlidingCache_CacheOperations()
     public void SlidingCache_TestExpire()
     {
         var dateTimeUtilsMock = new Mock<IDateTimeUtils>();
+        // Configure Mock with SetupGet since SlidingCache can be non-deterministic; don't use SetupSequence
         dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(UtcNow);
 
+        // Arrange
         var cache = new SlidingCache<int, string>(TimeSpan.FromMinutes(10),
             dateTimeProvider: dateTimeUtilsMock.Object);
 
         // Act
         cache.AddOrUpdate(1, "one");
 
+        // move the time forward 
+        var newDateTime = dateTimeUtilsMock.Object.UtcNow.AddMinutes(11);
+        dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(newDateTime);
+
+        // Ensure that the element has expired
+        cache.TryGetValue(1, out var value).Should().BeFalse($"Expected to not find the value, but found {value}");
+    }
+
+    [Fact]
+    public void SlidingCache_TestReturnExpiredItems()
+    {
+        var dateTimeUtilsMock = new Mock<IDateTimeUtils>();
+        // Configure Mock with SetupGet since SlidingCache can be non-deterministic; don't use SetupSequence
+        dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(UtcNow);
+
+        // Arrange
+        var cache = new SlidingCache<int, string>(TimeSpan.FromMinutes(10),
+            dateTimeProvider: dateTimeUtilsMock.Object, returnExpiredItems: true);
+
+        // Act
+        cache.AddOrUpdate(1, "one");
+
+        // move the time forward 
         var newDateTime = dateTimeUtilsMock.Object.UtcNow.AddMinutes(11);
         dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(newDateTime);
 
-        if (cache.TryGetValue(1, out var value))
-        {
-            Assert.True(false, $"Expected to not find the value, but found {value}");
-        }
+        // Ensure the expired item is returned from the cache
+        cache.TryGetValue(1, out _).Should().BeTrue($"Expected to return expired item");
     }
 
     [Fact]
     public void SlidingCache_TestAutoExpire()
     {
         var dateTimeUtilsMock = new Mock<IDateTimeUtils>();
+        // Configure Mock with SetupGet since SlidingCache can be non-deterministic; don't use SetupSequence
         dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(UtcNow);
 
         // Arrange
@@ -109,6 +134,7 @@ public void SlidingCache_TestNull()
     public void SlidingCache_TestMinNumberBeforeTests()
     {
         var dateTimeUtilsMock = new Mock<IDateTimeUtils>();
+        // Configure Mock with SetupGet since SlidingCache can be non-deterministic; don't use SetupSequence
         dateTimeUtilsMock.SetupGet(d => d.UtcNow).Returns(UtcNow);
 
         // Arrange