From e13b461cc8c8d9038c8113911354cc591c0ef265 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Thu, 7 Dec 2017 23:56:45 +0100 Subject: [PATCH 1/2] Fix GetQueryCache storing two different caches. --- .../Async/CacheTest/GetQueryCacheFixture.cs | 132 +++++++++++++++ .../CacheTest/GetQueryCacheFixture.cs | 157 ++++++++++++++++++ src/NHibernate/Impl/SessionFactoryImpl.cs | 13 +- 3 files changed, 299 insertions(+), 3 deletions(-) create mode 100644 src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs create mode 100644 src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs diff --git a/src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs b/src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs new file mode 100644 index 00000000000..ec86a0b8b24 --- /dev/null +++ b/src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs @@ -0,0 +1,132 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by AsyncGenerator. +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + + +using System; +using System.Collections; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Threading; +using NHibernate.Cache; +using NHibernate.Cfg; +using NUnit.Framework; +using Environment = NHibernate.Cfg.Environment; + +namespace NHibernate.Test.CacheTest +{ + using System.Threading.Tasks; + [TestFixture] + public class GetQueryCacheFixtureAsync : TestCase + { + protected override IList Mappings => new[] { "Simple.hbm.xml" }; + + protected override void Configure(Configuration configuration) + { + configuration.SetProperty(Environment.UseQueryCache, "true"); + configuration.SetProperty(Environment.CacheProvider, typeof(LockedCacheProvider).AssemblyQualifiedName); + } + + [Test] + public async Task RetrievedQueryCacheMatchesGloballyStoredOneAsync() + { + var region = "RetrievedQueryCacheMatchesGloballyStoredOne"; + LockedCache.Semaphore = new SemaphoreSlim(0); + try + { + var failures = new ConcurrentBag(); + var thread1 = new Thread( + () => + { + try + { + Sfi.GetQueryCache(region); + } + catch (Exception e) + { + failures.Add(e); + } + }); + var thread2 = new Thread( + () => + { + try + { + Sfi.GetQueryCache(region); + } + catch (Exception e) + { + failures.Add(e); + } + }); + thread1.Start(); + thread2.Start(); + + // Give some time to threads for reaching the wait, having all of them ready to do most of their job concurrently. + await (Task.Delay(100)); + // Let only one finish its job, it should be the one being stored in query cache dictionary. + LockedCache.Semaphore.Release(1); + // Give some time to released thread to finish its job. + await (Task.Delay(100)); + // Release other thread + LockedCache.Semaphore.Release(10); + + thread1.Join(); + thread2.Join(); + Assert.That(failures, Is.Empty, $"{failures.Count} thread(s) failed."); + } + finally + { + LockedCache.Semaphore.Dispose(); + LockedCache.Semaphore = null; + } + + var queryCache = Sfi.GetQueryCache(region).Cache; + var globalCache = Sfi.GetSecondLevelCacheRegion(region); + Assert.That(globalCache, Is.SameAs(queryCache)); + } + } + + public partial class LockedCache : ICache + { + + #region Void implementation + + public Task GetAsync(object key, CancellationToken cancellationToken) + { + return Task.FromResult(null); + } + + public Task PutAsync(object key, object value, CancellationToken cancellationToken) + { + return Task.CompletedTask; + } + + public Task RemoveAsync(object key, CancellationToken cancellationToken) + { + return Task.CompletedTask; + } + + public Task ClearAsync(CancellationToken cancellationToken) + { + return Task.CompletedTask; + } + + public Task LockAsync(object key, CancellationToken cancellationToken) + { + return Task.CompletedTask; + } + + public Task UnlockAsync(object key, CancellationToken cancellationToken) + { + return Task.CompletedTask; + } + + #endregion + } +} diff --git a/src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs b/src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs new file mode 100644 index 00000000000..d468f37b63d --- /dev/null +++ b/src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs @@ -0,0 +1,157 @@ +using System; +using System.Collections; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Threading; +using NHibernate.Cache; +using NHibernate.Cfg; +using NUnit.Framework; +using Environment = NHibernate.Cfg.Environment; + +namespace NHibernate.Test.CacheTest +{ + [TestFixture] + public class GetQueryCacheFixture : TestCase + { + protected override IList Mappings => new[] { "Simple.hbm.xml" }; + + protected override void Configure(Configuration configuration) + { + configuration.SetProperty(Environment.UseQueryCache, "true"); + configuration.SetProperty(Environment.CacheProvider, typeof(LockedCacheProvider).AssemblyQualifiedName); + } + + [Test] + public void RetrievedQueryCacheMatchesGloballyStoredOne() + { + var region = "RetrievedQueryCacheMatchesGloballyStoredOne"; + LockedCache.Semaphore = new SemaphoreSlim(0); + try + { + var failures = new ConcurrentBag(); + var thread1 = new Thread( + () => + { + try + { + Sfi.GetQueryCache(region); + } + catch (Exception e) + { + failures.Add(e); + } + }); + var thread2 = new Thread( + () => + { + try + { + Sfi.GetQueryCache(region); + } + catch (Exception e) + { + failures.Add(e); + } + }); + thread1.Start(); + thread2.Start(); + + // Give some time to threads for reaching the wait, having all of them ready to do most of their job concurrently. + Thread.Sleep(100); + // Let only one finish its job, it should be the one being stored in query cache dictionary. + LockedCache.Semaphore.Release(1); + // Give some time to released thread to finish its job. + Thread.Sleep(100); + // Release other thread + LockedCache.Semaphore.Release(10); + + thread1.Join(); + thread2.Join(); + Assert.That(failures, Is.Empty, $"{failures.Count} thread(s) failed."); + } + finally + { + LockedCache.Semaphore.Dispose(); + LockedCache.Semaphore = null; + } + + var queryCache = Sfi.GetQueryCache(region).Cache; + var globalCache = Sfi.GetSecondLevelCacheRegion(region); + Assert.That(globalCache, Is.SameAs(queryCache)); + } + } + + public partial class LockedCache : ICache + { + public static SemaphoreSlim Semaphore { get; set; } + + public LockedCache(string regionName) + { + RegionName = regionName; + Semaphore?.Wait(); + } + + #region Void implementation + + public object Get(object key) + { + return null; + } + + public void Put(object key, object value) + { + } + + public void Remove(object key) + { + } + + public void Clear() + { + } + + public void Destroy() + { + } + + public void Lock(object key) + { + } + + public void Unlock(object key) + { + } + + public long NextTimestamp() + { + return Timestamper.Next(); + } + + public int Timeout => Timestamper.OneMs * 60000; + + public string RegionName { get; } + + #endregion + } + + public class LockedCacheProvider : ICacheProvider + { + public ICache BuildCache(string regionName, IDictionary properties) + { + return new LockedCache(regionName); + } + + public long NextTimestamp() + { + return Timestamper.Next(); + } + + public void Start(IDictionary properties) + { + } + + public void Stop() + { + } + } +} diff --git a/src/NHibernate/Impl/SessionFactoryImpl.cs b/src/NHibernate/Impl/SessionFactoryImpl.cs index 78514bffb33..b5b5e2063b7 100644 --- a/src/NHibernate/Impl/SessionFactoryImpl.cs +++ b/src/NHibernate/Impl/SessionFactoryImpl.cs @@ -1029,14 +1029,21 @@ public IQueryCache GetQueryCache(string cacheRegion) return null; } - return queryCaches.GetOrAdd( + var wasAdded = false; + // The factory may be run concurrently by threads trying to get the same region. + // But the GetOrAdd will yield the same cache for all threads, so in case of add, use + // it to update allCacheRegions + var cache = queryCaches.GetOrAdd( cacheRegion, cr => { - IQueryCache currentQueryCache = settings.QueryCacheFactory.GetQueryCache(cacheRegion, updateTimestampsCache, settings, properties); - allCacheRegions[currentQueryCache.RegionName] = currentQueryCache.Cache; + var currentQueryCache = settings.QueryCacheFactory.GetQueryCache(cacheRegion, updateTimestampsCache, settings, properties); + wasAdded = true; return currentQueryCache; }); + if (wasAdded) + allCacheRegions[cache.RegionName] = cache.Cache; + return cache; } public void EvictQueries() From ea4b2d21410755c4f8eee49c08066b42a7d1112d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Fri, 8 Dec 2017 21:29:34 +0100 Subject: [PATCH 2/2] Fix GetQueryCache building two different caches for same region. --- .../Async/CacheTest/GetQueryCacheFixture.cs | 40 +------------- .../CacheTest/GetQueryCacheFixture.cs | 52 ++++--------------- .../Async/Impl/SessionFactoryImpl.cs | 9 ++-- src/NHibernate/Impl/SessionFactoryImpl.cs | 36 ++++++------- 4 files changed, 32 insertions(+), 105 deletions(-) diff --git a/src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs b/src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs index ec86a0b8b24..0efbe55d3e2 100644 --- a/src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs +++ b/src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs @@ -37,6 +37,7 @@ public async Task RetrievedQueryCacheMatchesGloballyStoredOneAsync() { var region = "RetrievedQueryCacheMatchesGloballyStoredOne"; LockedCache.Semaphore = new SemaphoreSlim(0); + LockedCache.CreationCount = 0; try { var failures = new ConcurrentBag(); @@ -89,44 +90,7 @@ public async Task RetrievedQueryCacheMatchesGloballyStoredOneAsync() var queryCache = Sfi.GetQueryCache(region).Cache; var globalCache = Sfi.GetSecondLevelCacheRegion(region); Assert.That(globalCache, Is.SameAs(queryCache)); + Assert.That(LockedCache.CreationCount, Is.EqualTo(1)); } } - - public partial class LockedCache : ICache - { - - #region Void implementation - - public Task GetAsync(object key, CancellationToken cancellationToken) - { - return Task.FromResult(null); - } - - public Task PutAsync(object key, object value, CancellationToken cancellationToken) - { - return Task.CompletedTask; - } - - public Task RemoveAsync(object key, CancellationToken cancellationToken) - { - return Task.CompletedTask; - } - - public Task ClearAsync(CancellationToken cancellationToken) - { - return Task.CompletedTask; - } - - public Task LockAsync(object key, CancellationToken cancellationToken) - { - return Task.CompletedTask; - } - - public Task UnlockAsync(object key, CancellationToken cancellationToken) - { - return Task.CompletedTask; - } - - #endregion - } } diff --git a/src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs b/src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs index d468f37b63d..8e5c1e4c938 100644 --- a/src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs +++ b/src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs @@ -26,6 +26,7 @@ public void RetrievedQueryCacheMatchesGloballyStoredOne() { var region = "RetrievedQueryCacheMatchesGloballyStoredOne"; LockedCache.Semaphore = new SemaphoreSlim(0); + LockedCache.CreationCount = 0; try { var failures = new ConcurrentBag(); @@ -78,60 +79,27 @@ public void RetrievedQueryCacheMatchesGloballyStoredOne() var queryCache = Sfi.GetQueryCache(region).Cache; var globalCache = Sfi.GetSecondLevelCacheRegion(region); Assert.That(globalCache, Is.SameAs(queryCache)); + Assert.That(LockedCache.CreationCount, Is.EqualTo(1)); } } - public partial class LockedCache : ICache + public class LockedCache : HashtableCache { public static SemaphoreSlim Semaphore { get; set; } - public LockedCache(string regionName) - { - RegionName = regionName; - Semaphore?.Wait(); - } - - #region Void implementation - - public object Get(object key) - { - return null; - } - - public void Put(object key, object value) - { - } - - public void Remove(object key) - { - } - - public void Clear() - { - } - - public void Destroy() - { - } - - public void Lock(object key) - { - } + private static int _creationCount; - public void Unlock(object key) + public static int CreationCount { + get => _creationCount; + set => _creationCount = value; } - public long NextTimestamp() + public LockedCache(string regionName) : base(regionName) { - return Timestamper.Next(); + Semaphore?.Wait(); + Interlocked.Increment(ref _creationCount); } - - public int Timeout => Timestamper.OneMs * 60000; - - public string RegionName { get; } - - #endregion } public class LockedCacheProvider : ICacheProvider diff --git a/src/NHibernate/Async/Impl/SessionFactoryImpl.cs b/src/NHibernate/Async/Impl/SessionFactoryImpl.cs index a82826eae6c..05b23b537ea 100644 --- a/src/NHibernate/Async/Impl/SessionFactoryImpl.cs +++ b/src/NHibernate/Async/Impl/SessionFactoryImpl.cs @@ -87,9 +87,9 @@ public sealed partial class SessionFactoryImpl : ISessionFactoryImplementor, IOb { queryCache.Destroy(); - foreach (IQueryCache cache in queryCaches.Values) + foreach (var cache in queryCaches.Values) { - cache.Destroy(); + cache.Value.Destroy(); } updateTimestampsCache.Destroy(); @@ -297,10 +297,9 @@ public sealed partial class SessionFactoryImpl : ISessionFactoryImplementor, IOb { if (settings.IsQueryCacheEnabled) { - IQueryCache currentQueryCache; - if (queryCaches.TryGetValue(cacheRegion, out currentQueryCache)) + if (queryCaches.TryGetValue(cacheRegion, out var currentQueryCache)) { - return currentQueryCache.ClearAsync(cancellationToken); + return currentQueryCache.Value.ClearAsync(cancellationToken); } } } diff --git a/src/NHibernate/Impl/SessionFactoryImpl.cs b/src/NHibernate/Impl/SessionFactoryImpl.cs index b5b5e2063b7..8c75b4e94a3 100644 --- a/src/NHibernate/Impl/SessionFactoryImpl.cs +++ b/src/NHibernate/Impl/SessionFactoryImpl.cs @@ -149,7 +149,7 @@ public void HandleEntityNotFound(string entityName, object id) private readonly IQueryCache queryCache; [NonSerialized] - private readonly ConcurrentDictionary queryCaches; + private readonly ConcurrentDictionary> queryCaches; [NonSerialized] private readonly SchemaExport schemaExport; [NonSerialized] @@ -381,7 +381,7 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings { updateTimestampsCache = new UpdateTimestampsCache(settings, properties); queryCache = settings.QueryCacheFactory.GetQueryCache(null, updateTimestampsCache, settings, properties); - queryCaches = new ConcurrentDictionary(); + queryCaches = new ConcurrentDictionary>(); } else { @@ -847,9 +847,9 @@ public void Close() { queryCache.Destroy(); - foreach (IQueryCache cache in queryCaches.Values) + foreach (var cache in queryCaches.Values) { - cache.Destroy(); + cache.Value.Destroy(); } updateTimestampsCache.Destroy(); @@ -1029,21 +1029,18 @@ public IQueryCache GetQueryCache(string cacheRegion) return null; } - var wasAdded = false; // The factory may be run concurrently by threads trying to get the same region. - // But the GetOrAdd will yield the same cache for all threads, so in case of add, use - // it to update allCacheRegions - var cache = queryCaches.GetOrAdd( + // But the GetOrAdd will yield the same lazy for all threads, so only one will + // initialize. https://stackoverflow.com/a/31637510/1178314 + return queryCaches.GetOrAdd( cacheRegion, - cr => - { - var currentQueryCache = settings.QueryCacheFactory.GetQueryCache(cacheRegion, updateTimestampsCache, settings, properties); - wasAdded = true; - return currentQueryCache; - }); - if (wasAdded) - allCacheRegions[cache.RegionName] = cache.Cache; - return cache; + cr => new Lazy( + () => + { + var currentQueryCache = settings.QueryCacheFactory.GetQueryCache(cr, updateTimestampsCache, settings, properties); + allCacheRegions[currentQueryCache.RegionName] = currentQueryCache.Cache; + return currentQueryCache; + })).Value; } public void EvictQueries() @@ -1069,10 +1066,9 @@ public void EvictQueries(string cacheRegion) { if (settings.IsQueryCacheEnabled) { - IQueryCache currentQueryCache; - if (queryCaches.TryGetValue(cacheRegion, out currentQueryCache)) + if (queryCaches.TryGetValue(cacheRegion, out var currentQueryCache)) { - currentQueryCache.Clear(); + currentQueryCache.Value.Clear(); } } }