Give-up sharing ICache per region

Author: fredericDelaporte

src/NHibernate.Test/CacheTest/BuildCacheFixture.cs

@@ -28,7 +28,7 @@ protected override void Configure(Configuration configuration)
 		}
 
 		[Theory]
-		public void CommonRegionHasOneUniqueCacheAndExpectedConcurrency(bool withPrefix)
+		public void CommonRegionHasExpectedConcurrency(bool withPrefix)
 		{
 			const string prefix = "Prefix";
 			const string region = "Common";
@@ -70,11 +70,6 @@ public void CommonRegionHasOneUniqueCacheAndExpectedConcurrency(bool withPrefix)
 						Assert.That(relatedAConcurrencyCache, Is.InstanceOf<NonstrictReadWriteCache>(), "Unexpected concurrency for RelatedA");
 						Assert.That(entityBConcurrencyCache, Is.InstanceOf<ReadOnlyCache>(), "Unexpected concurrency for EntityB");
 						Assert.That(relatedBConcurrencyCache, Is.InstanceOf<ReadWriteCache>(), "Unexpected concurrency for RelatedB");
-						Assert.That(entityACache, Is.EqualTo(commonRegionCache), "Unexpected cache for EntityA");
-						Assert.That(entityBCache, Is.EqualTo(commonRegionCache), "Unexpected cache for EntityB");
-						Assert.That(relatedACache, Is.EqualTo(commonRegionCache), "Unexpected cache for RelatedA");
-						Assert.That(relatedBCache, Is.EqualTo(commonRegionCache), "Unexpected cache for RelatedB");
-						Assert.That(queryCache, Is.EqualTo(commonRegionCache), "Unexpected cache for query cache");
 					});
 			}
 			finally

src/NHibernate/Async/Impl/SessionFactoryImpl.cs

@@ -85,8 +85,6 @@ public sealed partial class SessionFactoryImpl : ISessionFactoryImplementor, IOb
 
 			if (settings.IsQueryCacheEnabled)
 			{
-				queryCache.Destroy();
-
 				foreach (var cache in queryCaches.Values)
 				{
 					cache.Value.Destroy();

src/NHibernate/Cache/CacheFactory.cs

@@ -39,26 +39,29 @@ public static ICacheConcurrencyStrategy CreateCache(
 			Settings settings,
 			IDictionary<string, string> properties)
 		{
-			return CreateCache(
-				usage, name, mutable, settings,
-				r => settings.CacheProvider.BuildCache(r, properties));
+			var cache = CreateCache(
+				usage, name, settings,
+				(r, u) => settings.CacheProvider.BuildCache(r, properties));
+
+			if (cache != null && mutable && usage == ReadOnly)
+				Log.Warn("read-only cache configured for mutable: {0}", name);
+
+			return cache;
 		}
 
 		/// <summary>
 		/// Creates an <see cref="ICacheConcurrencyStrategy"/> from the parameters.
 		/// </summary>
 		/// <param name="usage">The name of the strategy that <see cref="ICacheProvider"/> should use for the class.</param>
-		/// <param name="name">The name of the class the strategy is being created for.</param>
-		/// <param name="mutable"><see langword="true" /> if the object being stored in the cache is mutable.</param>
+		/// <param name="name">The name of the cache region the strategy is being created for.</param>
 		/// <param name="settings">Used to retrieve the global cache region prefix.</param>
-		/// <param name="regionCacheGetter">The delegate for obtaining the <see cref="ICache" /> to use for the region.</param>
+		/// <param name="regionAndUsageCacheGetter">The delegate for obtaining the <see cref="ICache" /> to use for the region.</param>
 		/// <returns>An <see cref="ICacheConcurrencyStrategy"/> to use for this object in the <see cref="ICache"/>.</returns>
 		public static ICacheConcurrencyStrategy CreateCache(
 			string usage,
 			string name,
-			bool mutable,
 			Settings settings,
-			Func<string, ICache> regionCacheGetter)
+			Func<string, string, ICache> regionAndUsageCacheGetter)
 		{
 			if (usage == null || !settings.IsSecondLevelCacheEnabled) return null; //no cache
 
@@ -71,10 +74,6 @@ public static ICacheConcurrencyStrategy CreateCache(
 			switch (usage)
 			{
 				case ReadOnly:
-					if (mutable)
-					{
-						Log.Warn("read-only cache configured for mutable: {0}", name);
-					}
 					ccs = new ReadOnlyCache();
 					break;
 				case ReadWrite:
@@ -94,7 +93,7 @@ public static ICacheConcurrencyStrategy CreateCache(
 			ICache impl;
 			try
 			{
-				impl = regionCacheGetter(name);
+				impl = regionAndUsageCacheGetter(name, usage);
 			}
 			catch (CacheException e)
 			{

src/NHibernate/Impl/SessionFactoryImpl.cs

@@ -95,8 +95,8 @@ public void HandleEntityNotFound(string entityName, object id)
 		private static readonly IIdentifierGenerator UuidGenerator = new UUIDHexGenerator();
 
 		[NonSerialized]
-		private readonly ConcurrentDictionary<string, ICache> allCacheRegions =
-			new ConcurrentDictionary<string, ICache>();
+		private readonly ConcurrentDictionary<string, ConcurrentDictionary<string, ICache>> allCachePerRegionThenType =
+			new ConcurrentDictionary<string, ConcurrentDictionary<string, ICache>>();
 
 		[NonSerialized]
 		private readonly IDictionary<string, IClassMetadata> classMetadata;
@@ -148,6 +148,8 @@ public void HandleEntityNotFound(string entityName, object id)
 		[NonSerialized]
 		private readonly IQueryCache queryCache;
 
+		private const string QueryCacheType = "QueryCache";
+
 		[NonSerialized]
 		private readonly ConcurrentDictionary<string, Lazy<IQueryCache>> queryCaches;
 		[NonSerialized]
@@ -232,6 +234,7 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings
 
 			#region Persisters
 
+			var caches = new Dictionary<Tuple<string, string>, ICacheConcurrencyStrategy>();
 			entityPersisters = new Dictionary<string, IEntityPersister>();
 			implementorToEntityName = new Dictionary<System.Type, string>();
 
@@ -240,14 +243,11 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings
 			foreach (PersistentClass model in cfg.ClassMappings)
 			{
 				model.PrepareTemporaryTables(mapping, settings.Dialect);
-				var cacheRegion = model.RootClazz.CacheRegionName;
-				var strategy = model.CacheConcurrencyStrategy;
-				var cache = CacheFactory.CreateCache(
-					strategy,
-					cacheRegion,
+				var cache = GetCacheConcurrencyStrategy(
+					model.RootClazz.CacheRegionName,
+					model.CacheConcurrencyStrategy,
 					model.IsMutable,
-					settings,
-					GetOrBuild);
+					caches);
 				var cp = PersisterFactory.CreateClassPersister(model, cache, this, mapping);
 				entityPersisters[model.EntityName] = cp;
 				classMeta[model.EntityName] = cp.ClassMetadata;
@@ -263,13 +263,11 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings
 			collectionPersisters = new Dictionary<string, ICollectionPersister>();
 			foreach (Mapping.Collection model in cfg.CollectionMappings)
 			{
-				var cacheRegion = model.CacheRegionName;
-				var cache = CacheFactory.CreateCache(
+				var cache = GetCacheConcurrencyStrategy(
+					model.CacheRegionName,
 					model.CacheConcurrencyStrategy,
-					cacheRegion,
 					model.Owner.IsMutable,
-					settings,
-					GetOrBuild);
+					caches);
 				var persister = PersisterFactory.CreateCollectionPersister(model, cache, this);
 				collectionPersisters[model.Role] = persister;
 				IType indexType = persister.IndexType;
@@ -371,16 +369,17 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings
 
 			if (settings.IsQueryCacheEnabled)
 			{
-				var updateTimestampsCacheName = typeof(StandardQueryCache).Name;
-				updateTimestampsCache = new UpdateTimestampsCache(GetOrBuild(updateTimestampsCacheName));
+				var updateTimestampsCacheName = typeof(UpdateTimestampsCache).Name;
+				updateTimestampsCache = new UpdateTimestampsCache(BuildCache(updateTimestampsCacheName, updateTimestampsCacheName));
 				var queryCacheName = typeof(StandardQueryCache).FullName;
 				queryCache = settings.QueryCacheFactory.GetQueryCache(
 					queryCacheName,
 					updateTimestampsCache,
 					settings,
 					properties,
-					GetOrBuild(queryCacheName));
+					BuildCache(queryCacheName, QueryCacheType));
 				queryCaches = new ConcurrentDictionary<string, Lazy<IQueryCache>>();
+				queryCaches.TryAdd(queryCacheName, new Lazy<IQueryCache>(() => queryCache));
 			}
 			else
 			{
@@ -417,6 +416,30 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings
 			entityNotFoundDelegate = enfd;
 		}
 
+		private ICacheConcurrencyStrategy GetCacheConcurrencyStrategy(
+			string cacheRegion,
+			string strategy,
+			bool isMutable,
+			Dictionary<Tuple<string, string>, ICacheConcurrencyStrategy> caches)
+		{
+			var cacheKey = new Tuple<string, string>(cacheRegion, strategy);
+			if (!caches.TryGetValue(cacheKey, out var cache))
+			{
+				cache = CacheFactory.CreateCache(
+					strategy,
+					cacheRegion,
+					settings,
+					BuildCache);
+				if (cache != null)
+					caches.Add(cacheKey, cache);
+			}
+			
+			if (cache != null && isMutable && strategy == CacheFactory.ReadOnly)
+				log.Warn("read-only cache configured for mutable: {0}", name);
+
+			return cache;
+		}
+
 		public EventListeners EventListeners
 		{
 			get { return eventListeners; }
@@ -844,8 +867,6 @@ public void Close()
 
 			if (settings.IsQueryCacheEnabled)
 			{
-				queryCache.Destroy();
-
 				foreach (var cache in queryCaches.Values)
 				{
 					cache.Value.Destroy();
@@ -1034,34 +1055,41 @@ public UpdateTimestampsCache UpdateTimestampsCache
 
 		public IDictionary<string, ICache> GetAllSecondLevelCacheRegions()
 		{
-			// ToArray creates a moment in time snapshot
-			return allCacheRegions.ToArray().ToDictionary(kv => kv.Key, kv => kv.Value);
+			return
+				allCachePerRegionThenType
+					// ToArray creates a moment in time snapshot
+					.ToArray()
+					// Caches are not unique per region, take the first one.
+					.ToDictionary(kv => kv.Key, kv => kv.Value.Values.First());
 		}
 
 		public ICache GetSecondLevelCacheRegion(string regionName)
 		{
-			ICache result;
-			allCacheRegions.TryGetValue(regionName, out result);
-			return result;
+			if (!allCachePerRegionThenType.TryGetValue(regionName, out var result))
+				return null;
+			// Caches are not unique per region, take the first one.
+			return result.Values.First();
 		}
 
-		/// <summary>
-		/// Get an existing <see cref="ICache"/> or build a new one if not already existing.
-		/// </summary>
-		/// <param name="cacheRegion">The (unprefixed) cache region of the cache to get or build.</param>
-		/// <returns>A cache.</returns>
-		private ICache GetOrBuild(string cacheRegion)
+		private ICache BuildCache(string cacheRegion, string type)
 		{
-			// If run concurrently for the same region, this may built many caches for the same region.
+			// If run concurrently for the same region and type, this may built many caches for the same region and type.
 			// Currently only GetQueryCache may be run concurrently, and its implementation prevents
 			// concurrent creation call for the same region, so this will not happen.
 			// Otherwise the dictionary will have to be changed for using a lazy, see
 			// https://stackoverflow.com/a/31637510/1178314
 			var prefix = settings.CacheRegionPrefix;
 			if (!string.IsNullOrEmpty(prefix))
 				cacheRegion = prefix + '.' + cacheRegion;
-			return allCacheRegions.GetOrAdd(cacheRegion,
-				cr => settings.CacheProvider.BuildCache(cr, properties));
+			var cachesPerType = allCachePerRegionThenType.GetOrAdd(cacheRegion, cr => new ConcurrentDictionary<string, ICache>());
+			var cache = settings.CacheProvider.BuildCache(cacheRegion, properties);
+			if (!cachesPerType.TryAdd(type, cache))
+			{
+				cache.Destroy();
+				throw new InvalidOperationException($"A cache has already been built for region {cacheRegion} and type {type}.");
+			}
+
+			return cache;
 		}
 
 		/// <summary> Statistics SPI</summary>
@@ -1097,7 +1125,7 @@ public IQueryCache GetQueryCache(string cacheRegion)
 						updateTimestampsCache,
 						settings,
 						properties,
-						GetOrBuild(cr)))).Value;
+						BuildCache(cr, QueryCacheType)))).Value;
 		}
 
 		public void EvictQueries()