From a95f84334ab18a6f9aa69ec506a9d4b58ae34a82 Mon Sep 17 00:00:00 2001 From: Alastair Crabtree Date: Sat, 28 Dec 2019 01:31:05 +0000 Subject: [PATCH] bug: if you change the type of the item in the cache the old item should get eveicted fixes #46 --- .../CachingServiceMemoryCacheProviderTests.cs | 20 ++++ LazyCache/CachingService.cs | 105 ++++++++++++++---- 2 files changed, 103 insertions(+), 22 deletions(-) diff --git a/LazyCache.UnitTests/CachingServiceMemoryCacheProviderTests.cs b/LazyCache.UnitTests/CachingServiceMemoryCacheProviderTests.cs index c8df2ac..29c19c8 100644 --- a/LazyCache.UnitTests/CachingServiceMemoryCacheProviderTests.cs +++ b/LazyCache.UnitTests/CachingServiceMemoryCacheProviderTests.cs @@ -252,6 +252,15 @@ public void GetOrAddAndThenGetObjectReturnsCorrectType() Assert.IsNotNull(actual); } + [Test] + public void GetOrAddAndThenGetOrAddDifferentTypeDoesLastInWins() + { + var first = sut.GetOrAdd(TestKey, () => new object()); + var second = sut.GetOrAdd(TestKey, () => testObject); + Assert.IsNotNull(second); + Assert.IsInstanceOf(second); + } + [Test] public void GetOrAddAndThenGetValueObjectReturnsCorrectType() { @@ -268,6 +277,8 @@ public void GetOrAddAndThenGetWrongtypeObjectReturnsNull() Assert.IsNull(actual); } + + [Test] public void GetOrAddAsyncACancelledTaskDoesNotCacheIt() { @@ -315,6 +326,15 @@ public async Task GetOrAddAsyncAndThenGetAsyncObjectReturnsCorrectType() Assert.That(actual, Is.EqualTo(testObject)); } + [Test] + public async Task GetOrAddAsyncAndThenGetOrAddAsyncDifferentTypeDoesLastInWins() + { + var first = await sut.GetOrAddAsync(TestKey, () => Task.FromResult(new object())); + var second = await sut.GetOrAddAsync(TestKey, () => Task.FromResult(testObject)); + Assert.IsNotNull(second); + Assert.IsInstanceOf(second); + } + [Test] public async Task GetOrAddAsyncAndThenGetAsyncWrongObjectReturnsNull() { diff --git a/LazyCache/CachingService.cs b/LazyCache/CachingService.cs index 1bd5305..4623c5d 100644 --- a/LazyCache/CachingService.cs +++ b/LazyCache/CachingService.cs @@ -71,7 +71,7 @@ public virtual T Get(string key) var item = CacheProvider.Get(key); - return GetValueFromLazy(item); + return GetValueFromLazy(item, out _); } public virtual Task GetAsync(string key) @@ -80,7 +80,7 @@ public virtual Task GetAsync(string key) var item = CacheProvider.Get(key); - return GetValueFromAsyncLazy(item); + return GetValueFromAsyncLazy(item, out _); } public virtual T GetOrAdd(string key, Func addItemFactory) @@ -88,17 +88,19 @@ public virtual T GetOrAdd(string key, Func addItemFactory) ValidateKey(key); object cacheItem; + + object CacheFactory(ICacheEntry entry) => + new Lazy(() => + { + var result = addItemFactory(entry); + EnsureEvictionCallbackDoesNotReturnTheAsyncOrLazy(entry.PostEvictionCallbacks); + return result; + }); + locker.Wait(); //TODO: do we really need this? Could we just lock on the key? try { - cacheItem = CacheProvider.GetOrCreate(key, entry => - new Lazy(() => - { - var result = addItemFactory(entry); - EnsureEvictionCallbackDoesNotReturnTheAsyncOrLazy(entry.PostEvictionCallbacks); - return result; - }) - ); + cacheItem = CacheProvider.GetOrCreate(key, CacheFactory); } finally { @@ -107,7 +109,25 @@ public virtual T GetOrAdd(string key, Func addItemFactory) try { - return GetValueFromLazy(cacheItem); + var result = GetValueFromLazy(cacheItem, out var valueHasChangedType); + + // if we get a cache hit but for something with the wrong type we need to evict it, start again and cache the new item instead + if (valueHasChangedType) + { + CacheProvider.Remove(key); + locker.Wait(); //TODO: do we really need this? Could we just lock on the key? + try + { + cacheItem = CacheProvider.GetOrCreate(key, CacheFactory); + } + finally + { + locker.Release(); + } + result = GetValueFromLazy(cacheItem, out _ /* we just evicted so type change cannot happen this time */); + } + + return result; } catch //addItemFactory errored so do not cache the exception { @@ -138,16 +158,18 @@ public virtual async Task GetOrAddAsync(string key, Func + new AsyncLazy(() => + { + var result = addItemFactory(entry); + EnsureEvictionCallbackDoesNotReturnTheAsyncOrLazy(entry.PostEvictionCallbacks); + return result; + }); + try { - cacheItem = CacheProvider.GetOrCreate(key, entry => - new AsyncLazy(() => - { - var result = addItemFactory(entry); - EnsureEvictionCallbackDoesNotReturnTheAsyncOrLazy(entry.PostEvictionCallbacks); - return result; - }) - ); + cacheItem = CacheProvider.GetOrCreate(key, CacheFactory); } finally { @@ -156,7 +178,26 @@ await locker.WaitAsync() try { - var result = GetValueFromAsyncLazy(cacheItem); + var result = GetValueFromAsyncLazy(cacheItem, out var valueHasChangedType); + + // if we get a cache hit but for something with the wrong type we need to evict it, start again and cache the new item instead + if (valueHasChangedType) + { + CacheProvider.Remove(key); + await locker.WaitAsync() + .ConfigureAwait( + false); //TODO: do we really need to lock everything here - faster if we could lock on just the key? + try + { + cacheItem = CacheProvider.GetOrCreate(key, CacheFactory); + } + finally + { + locker.Release(); + } + result = GetValueFromAsyncLazy(cacheItem, out _ /* we just evicted so type change cannot happen this time */); + } + if (result.IsCanceled || result.IsFaulted) CacheProvider.Remove(key); @@ -170,8 +211,9 @@ await locker.WaitAsync() } } - protected virtual T GetValueFromLazy(object item) + protected virtual T GetValueFromLazy(object item, out bool valueHasChangedType) { + valueHasChangedType = false; switch (item) { case Lazy lazy: @@ -187,11 +229,21 @@ protected virtual T GetValueFromLazy(object item) return task.Result; } + // if they have cached something else with the same key we need to tell caller to reset the cached item + // although this is probably not the fastest this should not get called on the main use case + // where you just hit the first switch case above. + var itemsType = item?.GetType(); + if (itemsType != null && itemsType.IsGenericType && itemsType.GetGenericTypeDefinition() == typeof(Lazy<>)) + { + valueHasChangedType = true; + } + return default(T); } - protected virtual Task GetValueFromAsyncLazy(object item) + protected virtual Task GetValueFromAsyncLazy(object item, out bool valueHasChangedType) { + valueHasChangedType = false; switch (item) { case AsyncLazy asyncLazy: @@ -205,6 +257,15 @@ protected virtual Task GetValueFromAsyncLazy(object item) return Task.FromResult(variable); } + // if they have cached something else with the same key we need to tell caller to reset the cached item + // although this is probably not the fastest this should not get called on the main use case + // where you just hit the first switch case above. + var itemsType = item?.GetType(); + if (itemsType != null && itemsType.IsGenericType && itemsType.GetGenericTypeDefinition() == typeof(AsyncLazy<>)) + { + valueHasChangedType = true; + } + return Task.FromResult(default(T)); }