Merge pull request #196 from TurnerSoftware/detailed-cache-entry-status

Prevent returning expired entries

Author: Turnerj

src/CacheTower/CacheEntryStatus.cs

@@ -0,0 +1,25 @@
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace CacheTower
+{
+	/// <summary>
+	/// Describes the status of the entry - whether it is expired, stale or access to an entry was a miss.
+	/// </summary>
+	internal enum CacheEntryStatus
+	{
+		/// <summary>
+		/// When the cache entry is considered stale.
+		/// </summary>
+		Stale,
+		/// <summary>
+		/// When the cache entry is considered expired.
+		/// </summary>
+		Expired,
+		/// <summary>
+		/// When no cache entry was found.
+		/// </summary>
+		Miss
+	}
+}

src/CacheTower/CacheStack.cs

@@ -203,26 +203,34 @@ public async ValueTask<T> GetOrSetAsync<T>(string cacheKey, Func<T, Task<T>> get
 
 			var currentTime = DateTimeProvider.Now;
 			var cacheEntryPoint = await GetWithLayerIndexAsync<T>(cacheKey);
-			if (cacheEntryPoint != default && cacheEntryPoint.CacheEntry.Expiry > currentTime)
+			if (cacheEntryPoint != default)
 			{
-				var cacheEntry = cacheEntryPoint.CacheEntry;
-				if (settings.StaleAfter.HasValue && cacheEntry.GetStaleDate(settings) < currentTime)
+				if (cacheEntryPoint.CacheEntry.Expiry > currentTime)
 				{
-					//If the cache entry is stale, refresh the value in the background
-					_ = RefreshValueAsync(cacheKey, getter, settings, noExistingValueAvailable: false);
+					var cacheEntry = cacheEntryPoint.CacheEntry;
+					if (settings.StaleAfter.HasValue && cacheEntry.GetStaleDate(settings) < currentTime)
+					{
+						//If the cache entry is stale, refresh the value in the background
+						_ = RefreshValueAsync(cacheKey, getter, settings, CacheEntryStatus.Stale);
+					}
+					else if (cacheEntryPoint.LayerIndex > 0)
+					{
+						//If a lower-level cache is missing the latest data, attempt to set it in the background
+						_ = BackPopulateCacheAsync(cacheEntryPoint.LayerIndex, cacheKey, cacheEntry);
+					}
+
+					return cacheEntry.Value!;
 				}
-				else if (cacheEntryPoint.LayerIndex > 0)
+				else
 				{
-					//If a lower-level cache is missing the latest data, attempt to set it in the background
-					_ = BackPopulateCacheAsync(cacheEntryPoint.LayerIndex, cacheKey, cacheEntry);
+					//Refresh the value in the current thread because we only have expired data (we never return expired data)
+					return (await RefreshValueAsync(cacheKey, getter, settings, CacheEntryStatus.Expired))!.Value!;
 				}
-
-				return cacheEntry.Value!;
 			}
 			else
 			{
-				//Refresh the value in the current thread though because we have no old cache value, we have to lock and wait
-				return (await RefreshValueAsync(cacheKey, getter, settings, noExistingValueAvailable: true))!.Value!;
+				//Refresh the value in the current thread because we have no existing data
+				return (await RefreshValueAsync(cacheKey, getter, settings, CacheEntryStatus.Miss))!.Value!;
 			}
 		}
 
@@ -264,7 +272,7 @@ private async ValueTask BackPopulateCacheAsync<T>(int fromIndexExclusive, string
 			}
 		}
 
-		private async ValueTask<CacheEntry<T>?> RefreshValueAsync<T>(string cacheKey, Func<T, Task<T>> getter, CacheSettings settings, bool noExistingValueAvailable)
+		private async ValueTask<CacheEntry<T>?> RefreshValueAsync<T>(string cacheKey, Func<T, Task<T>> getter, CacheSettings settings, CacheEntryStatus entryStatus)
 		{
 			ThrowIfDisposed();
 
@@ -287,7 +295,7 @@ private async ValueTask BackPopulateCacheAsync<T>(int fromIndexExclusive, string
 				try
 				{
 					var previousEntry = await GetAsync<T>(cacheKey);
-					if (previousEntry != default && noExistingValueAvailable && previousEntry.Expiry < DateTimeProvider.Now)
+					if (previousEntry != default && entryStatus == CacheEntryStatus.Miss && previousEntry.Expiry >= DateTimeProvider.Now)
 					{
 						//The Cache Stack will always return an unexpired value if one exists.
 						//If we are told to refresh because one doesn't and we find one, we return the existing value, ignoring the refresh.
@@ -307,7 +315,11 @@ private async ValueTask BackPopulateCacheAsync<T>(int fromIndexExclusive, string
 
 						var value = await getter(oldValue!);
 						var refreshedEntry = new CacheEntry<T>(value, settings.TimeToLive);
-						var cacheUpdateType = noExistingValueAvailable ? CacheUpdateType.AddEntry : CacheUpdateType.AddOrUpdateEntry;
+						var cacheUpdateType = entryStatus switch
+						{
+							CacheEntryStatus.Miss => CacheUpdateType.AddEntry,
+							_ => CacheUpdateType.AddOrUpdateEntry
+						};
 						await InternalSetAsync(cacheKey, refreshedEntry, cacheUpdateType);
 
 						UnlockWaitingTasks(cacheKey, refreshedEntry);
@@ -321,7 +333,7 @@ private async ValueTask BackPopulateCacheAsync<T>(int fromIndexExclusive, string
 					throw;
 				}
 			}
-			else if (noExistingValueAvailable)
+			else if (entryStatus != CacheEntryStatus.Stale)
 			{
 				TaskCompletionSource<CacheEntry> completionSource;
 

tests/CacheTower.Tests/CacheStackTests.cs

@@ -451,5 +451,19 @@ public async Task GetOrSet_WaitingForRefresh()
 			Assert.AreEqual(42, await awaitingTasks[1]);
 			Assert.AreEqual(42, await awaitingTasks[2]);
 		}
+		[TestMethod]
+		public async Task GetOrSet_ExpiredCacheHit()
+		{
+			await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, Array.Empty<ICacheExtension>());
+			await cacheStack.SetAsync("GetOrSet_CacheHit", 17, TimeSpan.FromDays(-1));
+
+			var result = await cacheStack.GetOrSetAsync<int>("GetOrSet_CacheHit", (oldValue) =>
+			{
+				Assert.AreEqual(17, oldValue);
+				return Task.FromResult(27);
+			}, new CacheSettings(TimeSpan.FromDays(1)));
+
+			Assert.AreEqual(27, result);
+		}
 	}
 }