From a94f7e6ec053edb8d49dc8182779ef439aced701 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 21 May 2015 17:04:14 +1000 Subject: [PATCH] Fixes RepositoryBase.Get to not use a GetOrAdd with the IRuntimeCache and instead just use a linear lookup, set cache, return - this causes less confusion and also prevents any inner recursive key lookups within our cache which was causing YSODs. Added tests for the RepositoryBase's DeepCloneRuntimeCacheProvider (see code comment about what this does), updates other repo's to use the RuntimeCache property instead of accessing it via the RepositoryCache.RuntimeCache property. --- .../Cache/HttpRuntimeCacheProvider.cs | 2 + .../Cache/ObjectCacheRuntimeCacheProvider.cs | 2 + .../Repositories/ContentRepository.cs | 2 +- .../DeepCloneRuntimeCacheProvider.cs | 74 +++++++----- .../Repositories/DictionaryRepository.cs | 12 +- .../Repositories/LanguageRepository.cs | 8 +- .../Repositories/RepositoryBase.cs | 28 ++--- .../DeepCloneRuntimeCacheProviderTests.cs | 112 ++++++++++++++++++ src/Umbraco.Tests/Umbraco.Tests.csproj | 1 + 9 files changed, 182 insertions(+), 59 deletions(-) create mode 100644 src/Umbraco.Tests/Cache/DeepCloneRuntimeCacheProviderTests.cs diff --git a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs index dcd5198eec..88fee8ef3a 100644 --- a/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/HttpRuntimeCacheProvider.cs @@ -140,6 +140,7 @@ namespace Umbraco.Core.Cache var sliding = isSliding == false ? System.Web.Caching.Cache.NoSlidingExpiration : (timeout ?? System.Web.Caching.Cache.NoSlidingExpiration); lck.UpgradeToWriteLock(); + //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); } } @@ -197,6 +198,7 @@ namespace Umbraco.Core.Cache using (new WriteLock(_locker)) { + //NOTE: 'Insert' on System.Web.Caching.Cache actually does an add or update! _cache.Insert(cacheKey, result, dependency, absolute, sliding, priority, removedCallback); } } diff --git a/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs b/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs index dfe00449fc..76519a6722 100644 --- a/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Cache/ObjectCacheRuntimeCacheProvider.cs @@ -216,6 +216,7 @@ namespace Umbraco.Core.Cache var policy = GetPolicy(timeout, isSliding, removedCallback, dependentFiles); lck.UpgradeToWriteLock(); + //NOTE: This does an add or update MemoryCache.Set(cacheKey, result, policy); } } @@ -242,6 +243,7 @@ namespace Umbraco.Core.Cache if (value == null) return; // do not store null values (backward compat) var policy = GetPolicy(timeout, isSliding, removedCallback, dependentFiles); + //NOTE: This does an add or update MemoryCache.Set(cacheKey, result, policy); } diff --git a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs index beb442ae86..8069cb5adc 100644 --- a/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs @@ -658,7 +658,7 @@ namespace Umbraco.Core.Persistence.Repositories // then we can use that entity. Otherwise if it is not published (which can be the case // because we only store the 'latest' entries in the cache which might not be the published // version) - var fromCache = RepositoryCache.RuntimeCache.GetCacheItem(GetCacheIdKey(dto.NodeId)); + var fromCache = RuntimeCache.GetCacheItem(GetCacheIdKey(dto.NodeId)); //var fromCache = TryGetFromCache(dto.NodeId); if (fromCache != null && fromCache.Published) { diff --git a/src/Umbraco.Core/Persistence/Repositories/DeepCloneRuntimeCacheProvider.cs b/src/Umbraco.Core/Persistence/Repositories/DeepCloneRuntimeCacheProvider.cs index fa998f06a2..a87ec1e764 100644 --- a/src/Umbraco.Core/Persistence/Repositories/DeepCloneRuntimeCacheProvider.cs +++ b/src/Umbraco.Core/Persistence/Repositories/DeepCloneRuntimeCacheProvider.cs @@ -9,118 +9,128 @@ using Umbraco.Core.Models.EntityBase; namespace Umbraco.Core.Persistence.Repositories { /// - /// Ensures that all inserts and returns are a deep cloned copy of the item when - /// the item is IDeepCloneable + /// A wrapper for any IRuntimeCacheProvider that ensures that all inserts and returns + /// are a deep cloned copy of the item when the item is IDeepCloneable and that tracks changes are + /// reset if the object is TracksChangesEntityBase /// internal class DeepCloneRuntimeCacheProvider : IRuntimeCacheProvider { - private readonly IRuntimeCacheProvider _innerProvider; + internal IRuntimeCacheProvider InnerProvider { get; private set; } public DeepCloneRuntimeCacheProvider(IRuntimeCacheProvider innerProvider) { - _innerProvider = innerProvider; + InnerProvider = innerProvider; } #region Clear - doesn't require any changes public void ClearAllCache() { - _innerProvider.ClearAllCache(); + InnerProvider.ClearAllCache(); } public void ClearCacheItem(string key) { - _innerProvider.ClearCacheItem(key); + InnerProvider.ClearCacheItem(key); } public void ClearCacheObjectTypes(string typeName) { - _innerProvider.ClearCacheObjectTypes(typeName); + InnerProvider.ClearCacheObjectTypes(typeName); } public void ClearCacheObjectTypes() { - _innerProvider.ClearCacheObjectTypes(); + InnerProvider.ClearCacheObjectTypes(); } public void ClearCacheObjectTypes(Func predicate) { - _innerProvider.ClearCacheObjectTypes(predicate); + InnerProvider.ClearCacheObjectTypes(predicate); } public void ClearCacheByKeySearch(string keyStartsWith) { - _innerProvider.ClearCacheByKeySearch(keyStartsWith); + InnerProvider.ClearCacheByKeySearch(keyStartsWith); } public void ClearCacheByKeyExpression(string regexString) { - _innerProvider.ClearCacheByKeyExpression(regexString); + InnerProvider.ClearCacheByKeyExpression(regexString); } #endregion public IEnumerable GetCacheItemsByKeySearch(string keyStartsWith) { - return _innerProvider.GetCacheItemsByKeySearch(keyStartsWith) + return InnerProvider.GetCacheItemsByKeySearch(keyStartsWith) .Select(CheckCloneableAndTracksChanges); } public IEnumerable GetCacheItemsByKeyExpression(string regexString) { - return _innerProvider.GetCacheItemsByKeyExpression(regexString) + return InnerProvider.GetCacheItemsByKeyExpression(regexString) .Select(CheckCloneableAndTracksChanges); } public object GetCacheItem(string cacheKey) { - var item = _innerProvider.GetCacheItem(cacheKey); + var item = InnerProvider.GetCacheItem(cacheKey); return CheckCloneableAndTracksChanges(item); } public object GetCacheItem(string cacheKey, Func getCacheItem) { - return _innerProvider.GetCacheItem(cacheKey, () => + return InnerProvider.GetCacheItem(cacheKey, () => { - //Resolve the item but returned the cloned/reset item - var item = getCacheItem(); - return CheckCloneableAndTracksChanges(item); + var result = DictionaryCacheProviderBase.GetSafeLazy(getCacheItem); + var value = result.Value; // force evaluation now - this may throw if cacheItem throws, and then nothing goes into cache + if (value == null) return null; // do not store null values (backward compat) + + return CheckCloneableAndTracksChanges(value); }); } public object GetCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - return _innerProvider.GetCacheItem(cacheKey, () => + return InnerProvider.GetCacheItem(cacheKey, () => { - //Resolve the item but returned the cloned/reset item - var item = getCacheItem(); - return CheckCloneableAndTracksChanges(item); + var result = DictionaryCacheProviderBase.GetSafeLazy(getCacheItem); + var value = result.Value; // force evaluation now - this may throw if cacheItem throws, and then nothing goes into cache + if (value == null) return null; // do not store null values (backward compat) + + return CheckCloneableAndTracksChanges(value); }, timeout, isSliding, priority, removedCallback, dependentFiles); } public void InsertCacheItem(string cacheKey, Func getCacheItem, TimeSpan? timeout = null, bool isSliding = false, CacheItemPriority priority = CacheItemPriority.Normal, CacheItemRemovedCallback removedCallback = null, string[] dependentFiles = null) { - _innerProvider.InsertCacheItem(cacheKey, () => + InnerProvider.InsertCacheItem(cacheKey, () => { - //Resolve the item but returned the cloned/reset item - var item = getCacheItem(); - return CheckCloneableAndTracksChanges(item); + var result = DictionaryCacheProviderBase.GetSafeLazy(getCacheItem); + var value = result.Value; // force evaluation now - this may throw if cacheItem throws, and then nothing goes into cache + if (value == null) return null; // do not store null values (backward compat) + + return CheckCloneableAndTracksChanges(value); }, timeout, isSliding, priority, removedCallback, dependentFiles); } private static object CheckCloneableAndTracksChanges(object input) { - var entity = input as IDeepCloneable; - if (entity == null) return input; + var cloneable = input as IDeepCloneable; + if (cloneable != null) + { + input = cloneable.DeepClone(); + } - var cloned = entity.DeepClone(); //on initial construction we don't want to have dirty properties tracked // http://issues.umbraco.org/issue/U4-1946 - var tracksChanges = cloned as TracksChangesEntityBase; + var tracksChanges = input as IRememberBeingDirty; if (tracksChanges != null) { tracksChanges.ResetDirtyProperties(false); - return tracksChanges; + input = tracksChanges; } - return cloned; + + return input; } } } \ No newline at end of file diff --git a/src/Umbraco.Core/Persistence/Repositories/DictionaryRepository.cs b/src/Umbraco.Core/Persistence/Repositories/DictionaryRepository.cs index a8dac969d3..cf3193943b 100644 --- a/src/Umbraco.Core/Persistence/Repositories/DictionaryRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/DictionaryRepository.cs @@ -169,8 +169,8 @@ namespace Umbraco.Core.Persistence.Repositories entity.ResetDirtyProperties(); //Clear the cache entries that exist by uniqueid/item key - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.ItemKey)); - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.Key)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.ItemKey)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.Key)); } protected override void PersistDeletedItem(IDictionaryItem entity) @@ -181,8 +181,8 @@ namespace Umbraco.Core.Persistence.Repositories Database.Delete("WHERE id = @Id", new { Id = entity.Key }); //Clear the cache entries that exist by uniqueid/item key - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.ItemKey)); - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.Key)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.ItemKey)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.Key)); } private void RecursiveDelete(Guid parentId) @@ -196,8 +196,8 @@ namespace Umbraco.Core.Persistence.Repositories Database.Delete("WHERE id = @Id", new { Id = dto.UniqueId }); //Clear the cache entries that exist by uniqueid/item key - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(dto.Key)); - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(dto.UniqueId)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(dto.Key)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(dto.UniqueId)); } } diff --git a/src/Umbraco.Core/Persistence/Repositories/LanguageRepository.cs b/src/Umbraco.Core/Persistence/Repositories/LanguageRepository.cs index 1403d5e6c4..9a789ac3f6 100644 --- a/src/Umbraco.Core/Persistence/Repositories/LanguageRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/LanguageRepository.cs @@ -131,8 +131,8 @@ namespace Umbraco.Core.Persistence.Repositories entity.ResetDirtyProperties(); //Clear the cache entries that exist by key/iso - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.IsoCode)); - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.CultureName)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.IsoCode)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.CultureName)); } protected override void PersistDeletedItem(ILanguage entity) @@ -140,8 +140,8 @@ namespace Umbraco.Core.Persistence.Repositories base.PersistDeletedItem(entity); //Clear the cache entries that exist by key/iso - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.IsoCode)); - RepositoryCache.RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.CultureName)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.IsoCode)); + RuntimeCache.ClearCacheItem(GetCacheIdKey(entity.CultureName)); } #endregion diff --git a/src/Umbraco.Core/Persistence/Repositories/RepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/RepositoryBase.cs index 887766ddc0..f9088e1911 100644 --- a/src/Umbraco.Core/Persistence/Repositories/RepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/RepositoryBase.cs @@ -26,8 +26,7 @@ namespace Umbraco.Core.Persistence.Repositories //IMPORTANT: We will force the DeepCloneRuntimeCacheProvider to be used here which is a wrapper for the underlying // runtime cache to ensure that anything that can be deep cloned in/out is done so, this also ensures that our tracks // changes entities are reset. - //_cache = new CacheHelper(new DeepCloneRuntimeCacheProvider(cache.RuntimeCache), cache.StaticCache, cache.RequestCache); - _cache = cache; + _cache = new CacheHelper(new DeepCloneRuntimeCacheProvider(cache.RuntimeCache), cache.StaticCache, cache.RequestCache); } /// @@ -126,20 +125,17 @@ namespace Umbraco.Core.Persistence.Repositories /// public TEntity Get(TId id) { - return RuntimeCache.GetCacheItem( - GetCacheIdKey(id), () => - { - var entity = PerformGet(id); - if (entity == null) return null; - //on initial construction we don't want to have dirty properties tracked - // http://issues.umbraco.org/issue/U4-1946 - var asEntity = entity as TracksChangesEntityBase; - if (asEntity != null) - { - asEntity.ResetDirtyProperties(false); - } - return entity; - }); + var cacheKey = GetCacheIdKey(id); + var fromCache = RuntimeCache.GetCacheItem(cacheKey); + + if (fromCache != null) return fromCache; + + var entity = PerformGet(id); + if (entity == null) return null; + + RuntimeCache.InsertCacheItem(cacheKey, () => entity); + + return entity; } protected abstract IEnumerable PerformGetAll(params TId[] ids); diff --git a/src/Umbraco.Tests/Cache/DeepCloneRuntimeCacheProviderTests.cs b/src/Umbraco.Tests/Cache/DeepCloneRuntimeCacheProviderTests.cs new file mode 100644 index 0000000000..44deb1da40 --- /dev/null +++ b/src/Umbraco.Tests/Cache/DeepCloneRuntimeCacheProviderTests.cs @@ -0,0 +1,112 @@ +using System; +using System.Reflection; +using System.Web; +using NUnit.Framework; +using Umbraco.Core; +using Umbraco.Core.Cache; +using Umbraco.Core.Models; +using Umbraco.Core.Models.EntityBase; +using Umbraco.Core.Persistence.Repositories; + +namespace Umbraco.Tests.Cache +{ + [TestFixture] + public class DeepCloneRuntimeCacheProviderTests : RuntimeCacheProviderTests + { + private DeepCloneRuntimeCacheProvider _provider; + + protected override int GetTotalItemCount + { + get { return HttpRuntime.Cache.Count; } + } + + public override void Setup() + { + base.Setup(); + _provider = new DeepCloneRuntimeCacheProvider(new HttpRuntimeCacheProvider(HttpRuntime.Cache)); + } + + internal override ICacheProvider Provider + { + get { return _provider; } + } + + internal override IRuntimeCacheProvider RuntimeProvider + { + get { return _provider; } + } + + [Test] + public void Ensures_Cloned_And_Reset() + { + var original = new TestClass() + { + Name = "hello" + }; + Assert.IsTrue(original.IsDirty()); + + var val = _provider.GetCacheItem("test", () => original); + + Assert.AreNotEqual(original.CloneId, val.CloneId); + Assert.IsFalse(val.IsDirty()); + } + + [Test] + public void DoesNotCacheExceptions() + { + string value; + Assert.Throws(() => { value = (string)_provider.GetCacheItem("key", () => GetValue(1)); }); + Assert.Throws(() => { value = (string)_provider.GetCacheItem("key", () => GetValue(2)); }); + + // does not throw + value = (string)_provider.GetCacheItem("key", () => GetValue(3)); + Assert.AreEqual("succ3", value); + + // cache + value = (string)_provider.GetCacheItem("key", () => GetValue(4)); + Assert.AreEqual("succ3", value); + } + + private static string GetValue(int i) + { + Console.WriteLine("get" + i); + if (i < 3) + throw new Exception("fail"); + return "succ" + i; + } + + private class TestClass : TracksChangesEntityBase, IDeepCloneable + { + public TestClass() + { + CloneId = Guid.NewGuid(); + } + + private static readonly PropertyInfo WriterSelector = ExpressionHelper.GetPropertyInfo(x => x.Name); + + private string _name; + public string Name + { + get { return _name; } + set + { + SetPropertyValueAndDetectChanges(o => + { + _name = value; + return _name; + }, _name, WriterSelector); + } + } + + public Guid CloneId { get; set; } + + public object DeepClone() + { + var shallowClone = (TestClass)MemberwiseClone(); + DeepCloneHelper.DeepCloneRefProperties(this, shallowClone); + shallowClone.CloneId = Guid.NewGuid(); + return shallowClone; + } + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index 9190056d66..ee89db9684 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -171,6 +171,7 @@ +