diff --git a/src/CacheTower/CacheStack.cs b/src/CacheTower/CacheStack.cs index 43030ff5..81eaa0ea 100644 --- a/src/CacheTower/CacheStack.cs +++ b/src/CacheTower/CacheStack.cs @@ -192,28 +192,15 @@ public async ValueTask GetOrSetAsync(string cacheKey, Func> get throw new ArgumentNullException(nameof(getter)); } + var currentTime = DateTime.UtcNow; var cacheEntryPoint = await GetWithLayerIndexAsync(cacheKey); - if (cacheEntryPoint != default) + if (cacheEntryPoint != default && cacheEntryPoint.CacheEntry.Expiry > currentTime) { var cacheEntry = cacheEntryPoint.CacheEntry; - var currentTime = DateTime.UtcNow; if (cacheEntry.GetStaleDate(settings) < currentTime) { - if (cacheEntry.Expiry < currentTime) - { - //Refresh the value in the current thread though short circuit if we're unable to establish a lock - //If the lock isn't established, it will instead use the stale cache entry (even if past the allowed stale period) - var refreshedCacheEntry = await RefreshValueAsync(cacheKey, getter, settings, waitForRefresh: false); - if (refreshedCacheEntry != default) - { - cacheEntry = refreshedCacheEntry; - } - } - else - { - //Refresh the value in the background - _ = RefreshValueAsync(cacheKey, getter, settings, waitForRefresh: false); - } + //Refresh the value in the background + _ = RefreshValueAsync(cacheKey, getter, settings, noExistingValueAvailable: false); } else if (cacheEntryPoint.LayerIndex > 0) { @@ -226,7 +213,7 @@ public async ValueTask GetOrSetAsync(string cacheKey, Func> get 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, waitForRefresh: true)).Value; + return (await RefreshValueAsync(cacheKey, getter, settings, noExistingValueAvailable: true)).Value; } } @@ -268,7 +255,7 @@ private async ValueTask BackPopulateCacheAsync(int fromIndexExclusive, string } } - private async ValueTask> RefreshValueAsync(string cacheKey, Func> getter, CacheSettings settings, bool waitForRefresh) + private async ValueTask> RefreshValueAsync(string cacheKey, Func> getter, CacheSettings settings, bool noExistingValueAvailable) { ThrowIfDisposed(); @@ -290,12 +277,21 @@ private async ValueTask> RefreshValueAsync(string cacheKey, Fun { try { - return await Extensions.WithRefreshAsync(cacheKey, async () => + var previousEntry = await GetAsync(cacheKey); + if (previousEntry != default && noExistingValueAvailable && previousEntry.Expiry < DateTime.UtcNow) { - var previousEntry = await GetAsync(cacheKey); + //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. + //This can happen due to the race condition of getting the values out of the cache. + //We can only do any of this because we have the local lock. + UnlockWaitingTasks(cacheKey, previousEntry); + return previousEntry; + } + return await Extensions.WithRefreshAsync(cacheKey, async () => + { var oldValue = default(T); - if (previousEntry != null) + if (previousEntry != default) { oldValue = previousEntry.Value; } @@ -314,7 +310,7 @@ private async ValueTask> RefreshValueAsync(string cacheKey, Fun throw; } } - else if (waitForRefresh) + else if (noExistingValueAvailable) { var delayedResultSource = new TaskCompletionSource(); diff --git a/tests/CacheTower.Tests/CacheStackTests.cs b/tests/CacheTower.Tests/CacheStackTests.cs index 8b43b814..46340a35 100644 --- a/tests/CacheTower.Tests/CacheStackTests.cs +++ b/tests/CacheTower.Tests/CacheStackTests.cs @@ -1,11 +1,10 @@ using CacheTower.Providers.Memory; using System; using System.Collections.Generic; -using System.Linq; -using System.Text; using System.Threading.Tasks; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; +using System.Threading; namespace CacheTower.Tests { @@ -240,26 +239,25 @@ public async Task GetOrSet_CacheHit() Assert.AreEqual(17, result); } [TestMethod] - public async Task GetOrSet_CacheHitBackgroundRefresh() + public async Task GetOrSet_StaleCacheHit() { await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, Array.Empty()); - var cacheEntry = new CacheEntry(17, DateTime.UtcNow.AddDays(1)); - await cacheStack.SetAsync("GetOrSet_CacheHitBackgroundRefresh", cacheEntry); + var cacheEntry = new CacheEntry(17, DateTime.UtcNow.AddDays(2)); + await cacheStack.SetAsync("GetOrSet_StaleCacheHit", cacheEntry); - var waitingOnBackgroundTask = new TaskCompletionSource(); + var refreshWaitSource = new TaskCompletionSource(); - var result = await cacheStack.GetOrSetAsync("GetOrSet_CacheHitBackgroundRefresh", (oldValue) => + var result = await cacheStack.GetOrSetAsync("GetOrSet_StaleCacheHit", (oldValue) => { - waitingOnBackgroundTask.TrySetResult(27); + Assert.AreEqual(17, oldValue); + refreshWaitSource.TrySetResult(true); return Task.FromResult(27); }, new CacheSettings(TimeSpan.FromDays(2), TimeSpan.Zero)); Assert.AreEqual(17, result); - await waitingOnBackgroundTask.Task; - //Give 400ms to return the value and set it to the MemoryCacheLayer - await Task.Delay(400); + await Task.WhenAny(refreshWaitSource.Task, Task.Delay(TimeSpan.FromSeconds(5))); - var refetchedResult = await cacheStack.GetAsync("GetOrSet_CacheHitBackgroundRefresh"); + var refetchedResult = await cacheStack.GetAsync("GetOrSet_StaleCacheHit"); Assert.AreEqual(27, refetchedResult.Value); } [TestMethod] @@ -287,52 +285,33 @@ public async Task GetOrSet_BackPropagatesToEarlierCacheLayers() Assert.IsNull(await layer3.GetAsync("GetOrSet_BackPropagatesToEarlierCacheLayers")); } [TestMethod] - public async Task GetOrSet_CacheHitButAllowedStalePoint() + public async Task GetOrSet_ConcurrentStaleCacheHits_OnlyOneRefresh() { await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, Array.Empty()); - var cacheEntry = new CacheEntry(17, DateTime.UtcNow.AddDays(-1)); - await cacheStack.SetAsync("GetOrSet_CacheHitButAllowedStalePoint", cacheEntry); + var cacheEntry = new CacheEntry(23, DateTime.UtcNow.AddDays(2)); + await cacheStack.SetAsync("GetOrSet_ConcurrentStaleCacheHits_OnlyOneRefresh", cacheEntry); - var result = await cacheStack.GetOrSetAsync("GetOrSet_CacheHitButAllowedStalePoint", (oldValue) => - { - return Task.FromResult(27); - }, new CacheSettings(TimeSpan.FromDays(1), TimeSpan.Zero)); - Assert.AreEqual(27, result); - } - [TestMethod] - public async Task GetOrSet_ConcurrentStaleCacheHits() - { - await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, Array.Empty()); - var cacheEntry = new CacheEntry(23, DateTime.UtcNow.AddDays(-2)); - await cacheStack.SetAsync("GetOrSet_ConcurrentStaleCacheHits", cacheEntry); - - var request1LockSource = new TaskCompletionSource(); - var request2StartLockSource = new TaskCompletionSource(); + var refreshWaitSource = new TaskCompletionSource(); + var getterCallCount = 0; - //Request 1 gets the lock on the refresh and ends up being tied up due to the TaskCompletionSource - var request1Task = cacheStack.GetOrSetAsync("GetOrSet_ConcurrentStaleCacheHits", async (oldValue) => + Parallel.For(0, 100, async v => { - request2StartLockSource.SetResult(true); - await request1LockSource.Task; - return 99; - }, new CacheSettings(TimeSpan.FromDays(2), TimeSpan.Zero)); - - await request2StartLockSource.Task; - - //Request 2 sees there is a lock already and because we still at least have old data, rather than wait - //it is given the old cache data even though we are past the point where even stale data should be removed - var request2Result = await cacheStack.GetOrSetAsync("GetOrSet_ConcurrentStaleCacheHits", (oldValue) => - { - return Task.FromResult(99); - }, new CacheSettings(TimeSpan.FromDays(2), TimeSpan.Zero)); - - //Unlock Request 1 to to continue - request1LockSource.SetResult(true); - //Wait for Request 1 to complete so we get the new data - var request1Result = await request1Task; - - Assert.AreEqual(99, request1Result); - Assert.AreEqual(23, request2Result); + await cacheStack.GetOrSetAsync( + "GetOrSet_ConcurrentStaleCacheHits_OnlyOneRefresh", + async _ => + { + await Task.Delay(250); + Interlocked.Increment(ref getterCallCount); + refreshWaitSource.TrySetResult(true); + return 27; + }, + new CacheSettings(TimeSpan.FromDays(2), TimeSpan.Zero) + ); + }); + + await Task.WhenAny(refreshWaitSource.Task, Task.Delay(TimeSpan.FromSeconds(5))); + + Assert.AreEqual(1, getterCallCount); } [TestMethod, ExpectedException(typeof(ObjectDisposedException))] public async Task GetOrSet_ThrowsOnUseAfterDisposal() diff --git a/tests/CacheTower.Tests/Extensions/Redis/RedisLockExtensionTests.cs b/tests/CacheTower.Tests/Extensions/Redis/RedisLockExtensionTests.cs index 454e08b9..9186c041 100644 --- a/tests/CacheTower.Tests/Extensions/Redis/RedisLockExtensionTests.cs +++ b/tests/CacheTower.Tests/Extensions/Redis/RedisLockExtensionTests.cs @@ -104,7 +104,12 @@ await extension.WithRefreshAsync("TestKey", () => new ValueTask>(cacheEntry), new CacheSettings(TimeSpan.FromDays(1))); var succeedingTask = await Task.WhenAny(completionSource.Task, Task.Delay(TimeSpan.FromSeconds(10))); - Assert.AreEqual(completionSource.Task, succeedingTask, "Subscriber response took too long"); + if (!succeedingTask.Equals(completionSource.Task)) + { + RedisHelper.DebugInfo(connection); + Assert.Fail("Subscriber response took too long"); + } + Assert.IsTrue(completionSource.Task.Result, "Subscribers were not notified about the refreshed value"); } @@ -142,7 +147,12 @@ public async Task ObservedLockSingle() await connection.GetSubscriber().PublishAsync("CacheTower.CacheLock", "TestKey"); var succeedingTask = await Task.WhenAny(refreshTask, Task.Delay(TimeSpan.FromSeconds(10))); - Assert.AreEqual(refreshTask, succeedingTask, "Refresh has timed out - something has gone very wrong"); + if (!succeedingTask.Equals(refreshTask)) + { + RedisHelper.DebugInfo(connection); + Assert.Fail("Refresh has timed out - something has gone very wrong"); + } + cacheStackMock.Verify(c => c.GetAsync("TestKey"), Times.Exactly(2), "Two checks to the cache stack are expected"); } @@ -189,7 +199,12 @@ public async Task ObservedLockMultiple() var whenAllRefreshesTask = Task.WhenAll(refreshTask1, refreshTask2); var succeedingTask = await Task.WhenAny(whenAllRefreshesTask, Task.Delay(TimeSpan.FromSeconds(10))); - Assert.AreEqual(whenAllRefreshesTask, succeedingTask, "Refresh has timed out - something has gone very wrong"); + if (!succeedingTask.Equals(whenAllRefreshesTask)) + { + RedisHelper.DebugInfo(connection); + Assert.Fail("Refresh has timed out - something has gone very wrong"); + } + cacheStackMock.Verify(c => c.GetAsync("TestKey"), Times.Exactly(4), "Two checks to the cache stack are expected"); } } diff --git a/tests/CacheTower.Tests/Extensions/Redis/RedisRemoteEvictionExtensionTests.cs b/tests/CacheTower.Tests/Extensions/Redis/RedisRemoteEvictionExtensionTests.cs index b6a70538..95f80747 100644 --- a/tests/CacheTower.Tests/Extensions/Redis/RedisRemoteEvictionExtensionTests.cs +++ b/tests/CacheTower.Tests/Extensions/Redis/RedisRemoteEvictionExtensionTests.cs @@ -117,7 +117,12 @@ public async Task RemoteEvictionOccursOnLocalEviction() await extensionOne.OnCacheEvictionAsync("TestKey"); var succeedingTask = await Task.WhenAny(completionSource.Task, Task.Delay(TimeSpan.FromSeconds(10))); - Assert.AreEqual(completionSource.Task, succeedingTask, "Subscriber response took too long"); + if (!succeedingTask.Equals(completionSource.Task)) + { + RedisHelper.DebugInfo(connection); + Assert.Fail("Subscriber response took too long"); + } + Assert.IsTrue(completionSource.Task.Result, "Subscribers were not notified about the refreshed value"); await Task.Delay(500); @@ -159,7 +164,12 @@ public async Task RemoteFlush() await extensionOne.OnCacheFlushAsync(); var succeedingTask = await Task.WhenAny(completionSource.Task, Task.Delay(TimeSpan.FromSeconds(10))); - Assert.AreEqual(completionSource.Task, succeedingTask, "Subscriber response took too long"); + if (!succeedingTask.Equals(completionSource.Task)) + { + RedisHelper.DebugInfo(connection); + Assert.Fail("Subscriber response took too long"); + } + Assert.IsTrue(completionSource.Task.Result, "Subscribers were not notified about the flush"); await Task.Delay(500); diff --git a/tests/CacheTower.Tests/Utils/RedisHelper.cs b/tests/CacheTower.Tests/Utils/RedisHelper.cs index beeb802a..d638f8d4 100644 --- a/tests/CacheTower.Tests/Utils/RedisHelper.cs +++ b/tests/CacheTower.Tests/Utils/RedisHelper.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Text; using System.Threading.Tasks; using StackExchange.Redis; @@ -24,5 +25,11 @@ public static void FlushDatabase() { GetConnection().GetServer(Endpoint).FlushDatabase(); } + + public static void DebugInfo(IConnectionMultiplexer connection) + { + Debug.WriteLine(connection.GetStatus()); + Debug.WriteLine(connection.GetCounters().ToString()); + } } }