From c62f5b960073c8c90b4fd8c8024db996a09b7054 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 11 Jan 2021 12:15:53 +0100 Subject: [PATCH 1/4] add a failing test --- ...oft.Extensions.Caching.Memory.Tests.csproj | 5 ++++ .../tests/TokenExpirationTests.cs | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj index 7046da24da6d6d..d6f0cc0857de08 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj @@ -5,6 +5,11 @@ true + + + + diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs index 3da2515de8789e..af28ca6da26892 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs @@ -207,6 +207,33 @@ public void TokenExpiresOnRegister() Assert.Null(result); } + [Fact] + public async Task PostEvictionCallbacksGetInvokedWhenMemoryCacheEntriesExpireWithAnActiveChangeToken() + { + var cache = new MemoryCache(new MemoryCacheOptions()); + var key = new object(); + + var cts = new CancellationTokenSource(TimeSpan.FromSeconds(1)); + var tcs = new TaskCompletionSource(); + + cache.Set(key, new object(), new MemoryCacheEntryOptions + { + ExpirationTokens = { new Microsoft.Extensions.Primitives.CancellationChangeToken(cts.Token) }, + PostEvictionCallbacks = { new PostEvictionCallbackRegistration { EvictionCallback = OnEntryEvicted } }, + }); + + Assert.True(cache.TryGetValue(key, out _)); + + await Task.Run(() => tcs.Task.TimeoutAfter(TimeSpan.FromSeconds(10))); + + Assert.False(cache.TryGetValue(key, out _)); + + void OnEntryEvicted(object key, object value, EvictionReason reason, object state) + { + tcs.TrySetResult(new object()); + } + } + internal class TestToken : IChangeToken { private bool _hasChanged; From 2e8cdcf61407b4f86ee85078f2aebcc16b346dc2 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 11 Jan 2021 14:41:47 +0100 Subject: [PATCH 2/4] fix the actual bug (this was not a CacheEntry but a CacheEntryTokens instance here) --- .../src/CacheEntry.CacheEntryTokens.cs | 4 ++-- .../Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs index 5800a40118809a..94e7144bb94996 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.CacheEntryTokens.cs @@ -23,7 +23,7 @@ private sealed class CacheEntryTokens internal List ExpirationTokens => _expirationTokens ??= new List(); internal List PostEvictionCallbacks => _postEvictionCallbacks ??= new List(); - internal void AttachTokens() + internal void AttachTokens(CacheEntry cacheEntry) { if (_expirationTokens != null) { @@ -35,7 +35,7 @@ internal void AttachTokens() if (expirationToken.ActiveChangeCallbacks) { _expirationTokenRegistrations ??= new List(1); - IDisposable registration = expirationToken.RegisterChangeCallback(ExpirationCallback, this); + IDisposable registration = expirationToken.RegisterChangeCallback(ExpirationCallback, cacheEntry); _expirationTokenRegistrations.Add(registration); } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 606f0adac088ee..8ac62a6524e16d 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -199,7 +199,7 @@ bool FullCheck(in DateTimeOffset offset) } } - internal void AttachTokens() => _tokens?.AttachTokens(); + internal void AttachTokens() => _tokens?.AttachTokens(this); private static void ExpirationTokensExpired(object obj) { From 32f43c52d0e69b6b239694a98336e3fc12a7ec59 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 11 Jan 2021 17:10:33 +0100 Subject: [PATCH 3/4] address code review feedback --- .../tests/TokenExpirationTests.cs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs index af28ca6da26892..9c8057da66775f 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs @@ -210,7 +210,7 @@ public void TokenExpiresOnRegister() [Fact] public async Task PostEvictionCallbacksGetInvokedWhenMemoryCacheEntriesExpireWithAnActiveChangeToken() { - var cache = new MemoryCache(new MemoryCacheOptions()); + using var cache = new MemoryCache(new MemoryCacheOptions()); var key = new object(); var cts = new CancellationTokenSource(TimeSpan.FromSeconds(1)); @@ -218,20 +218,16 @@ public async Task PostEvictionCallbacksGetInvokedWhenMemoryCacheEntriesExpireWit cache.Set(key, new object(), new MemoryCacheEntryOptions { - ExpirationTokens = { new Microsoft.Extensions.Primitives.CancellationChangeToken(cts.Token) }, - PostEvictionCallbacks = { new PostEvictionCallbackRegistration { EvictionCallback = OnEntryEvicted } }, + ExpirationTokens = { new CancellationChangeToken(cts.Token) }, + PostEvictionCallbacks = { new PostEvictionCallbackRegistration { + EvictionCallback = (key, value, reason, state) => tcs.TrySetResult(new object()) } }, }); Assert.True(cache.TryGetValue(key, out _)); - await Task.Run(() => tcs.Task.TimeoutAfter(TimeSpan.FromSeconds(10))); + await tcs.Task.TimeoutAfter(TimeSpan.FromSeconds(10)); Assert.False(cache.TryGetValue(key, out _)); - - void OnEntryEvicted(object key, object value, EvictionReason reason, object state) - { - tcs.TrySetResult(new object()); - } } internal class TestToken : IChangeToken From 76d319c0c4ea57e9706805aa84ffafa8b209cdfc Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 11 Jan 2021 17:56:49 +0100 Subject: [PATCH 4/4] address code review feedback: - remove the dependency to TaskTimeoutExtensions - don't wait 1s, call Cancel in explicit way --- ...oft.Extensions.Caching.Memory.Tests.csproj | 5 ----- .../tests/TokenExpirationTests.cs | 20 ++++++++++++------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj index d6f0cc0857de08..7046da24da6d6d 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj @@ -5,11 +5,6 @@ true - - - - diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs index 9c8057da66775f..182d53cf2e8b98 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/TokenExpirationTests.cs @@ -208,25 +208,31 @@ public void TokenExpiresOnRegister() } [Fact] - public async Task PostEvictionCallbacksGetInvokedWhenMemoryCacheEntriesExpireWithAnActiveChangeToken() + public void PostEvictionCallbacksGetInvokedWhenMemoryCacheEntriesExpireWithAnActiveChangeToken() { using var cache = new MemoryCache(new MemoryCacheOptions()); var key = new object(); - var cts = new CancellationTokenSource(TimeSpan.FromSeconds(1)); - var tcs = new TaskCompletionSource(); + var cts = new CancellationTokenSource(); + var callbackInvoked = new ManualResetEvent(false); cache.Set(key, new object(), new MemoryCacheEntryOptions { ExpirationTokens = { new CancellationChangeToken(cts.Token) }, - PostEvictionCallbacks = { new PostEvictionCallbackRegistration { - EvictionCallback = (key, value, reason, state) => tcs.TrySetResult(new object()) } }, + PostEvictionCallbacks = + { + new PostEvictionCallbackRegistration() + { + EvictionCallback = (key, value, reason, state) => ((ManualResetEvent)state).Set(), + State = callbackInvoked + } + } }); Assert.True(cache.TryGetValue(key, out _)); - await tcs.Task.TimeoutAfter(TimeSpan.FromSeconds(10)); - + cts.Cancel(); + Assert.True(callbackInvoked.WaitOne(TimeSpan.FromSeconds(10))); Assert.False(cache.TryGetValue(key, out _)); }