Enhancements to SlidingCache<> for Improved Thread Safety and Perform…

…ance (#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]>

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