bug: if you change the type of the item in the cache the old item should get eveicted

fixes #46

Author: alastairtree

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<ComplexTestObject>(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<ComplexTestObject>(second);
+        }
+
         [Test]
         public async Task GetOrAddAsyncAndThenGetAsyncWrongObjectReturnsNull()
         {

LazyCache/CachingService.cs

@@ -71,7 +71,7 @@ public virtual T Get<T>(string key)
 
             var item = CacheProvider.Get(key);
 
-            return GetValueFromLazy<T>(item);
+            return GetValueFromLazy<T>(item, out _);
         }
 
         public virtual Task<T> GetAsync<T>(string key)
@@ -80,25 +80,27 @@ public virtual Task<T> GetAsync<T>(string key)
 
             var item = CacheProvider.Get(key);
 
-            return GetValueFromAsyncLazy<T>(item);
+            return GetValueFromAsyncLazy<T>(item, out _);
         }
 
         public virtual T GetOrAdd<T>(string key, Func<ICacheEntry, T> addItemFactory)
         {
             ValidateKey(key);
 
             object cacheItem;
+
+            object CacheFactory(ICacheEntry entry) =>
+                new Lazy<T>(() =>
+                {
+                    var result = addItemFactory(entry);
+                    EnsureEvictionCallbackDoesNotReturnTheAsyncOrLazy<T>(entry.PostEvictionCallbacks);
+                    return result;
+                });
+
             locker.Wait(); //TODO: do we really need this? Could we just lock on the key?
             try
             {
-                cacheItem = CacheProvider.GetOrCreate<object>(key, entry =>
-                    new Lazy<T>(() =>
-                    {
-                        var result = addItemFactory(entry);
-                        EnsureEvictionCallbackDoesNotReturnTheAsyncOrLazy<T>(entry.PostEvictionCallbacks);
-                        return result;
-                    })
-                );
+                cacheItem = CacheProvider.GetOrCreate<object>(key, CacheFactory);
             }
             finally
             {
@@ -107,7 +109,25 @@ public virtual T GetOrAdd<T>(string key, Func<ICacheEntry, T> addItemFactory)
 
             try
             {
-                return GetValueFromLazy<T>(cacheItem);
+                var result =  GetValueFromLazy<T>(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<object>(key, CacheFactory);
+                    }
+                    finally
+                    {
+                        locker.Release();
+                    }
+                    result = GetValueFromLazy<T>(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<T> GetOrAddAsync<T>(string key, Func<ICacheEntry, Task
             await locker.WaitAsync()
                 .ConfigureAwait(
                     false); //TODO: do we really need to lock everything here - faster if we could lock on just the key?
+
+            object CacheFactory(ICacheEntry entry) =>
+                new AsyncLazy<T>(() =>
+                {
+                    var result = addItemFactory(entry);
+                    EnsureEvictionCallbackDoesNotReturnTheAsyncOrLazy<T>(entry.PostEvictionCallbacks);
+                    return result;
+                });
+
             try
             {
-                cacheItem = CacheProvider.GetOrCreate<object>(key, entry =>
-                    new AsyncLazy<T>(() =>
-                    {
-                        var result = addItemFactory(entry);
-                        EnsureEvictionCallbackDoesNotReturnTheAsyncOrLazy<T>(entry.PostEvictionCallbacks);
-                        return result;
-                    })
-                );
+                cacheItem = CacheProvider.GetOrCreate<object>(key, CacheFactory);
             }
             finally
             {
@@ -156,7 +178,26 @@ await locker.WaitAsync()
 
             try
             {
-                var result = GetValueFromAsyncLazy<T>(cacheItem);
+                var result = GetValueFromAsyncLazy<T>(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<object>(key, CacheFactory);
+                    }
+                    finally
+                    {
+                        locker.Release();
+                    }
+                    result = GetValueFromAsyncLazy<T>(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<T>(object item)
+        protected virtual T GetValueFromLazy<T>(object item, out bool valueHasChangedType)
         {
+            valueHasChangedType = false;
             switch (item)
             {
                 case Lazy<T> lazy:
@@ -187,11 +229,21 @@ protected virtual T GetValueFromLazy<T>(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<T> GetValueFromAsyncLazy<T>(object item)
+        protected virtual Task<T> GetValueFromAsyncLazy<T>(object item, out bool valueHasChangedType)
         {
+            valueHasChangedType = false; 
             switch (item)
             {
                 case AsyncLazy<T> asyncLazy:
@@ -205,6 +257,15 @@ protected virtual Task<T> GetValueFromAsyncLazy<T>(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));
         }