Fix GetQueryCache storing two different caches.

[fredericDelaporte]

SessionFactoryImpl stores built QueryCache in a dictionary and yields it. But it also stores its inner ICache in another global cache dictionary. It does this in the factory method for ConcurrentDictionary.GetOrAdd.

This method may be executed by concurrent threads for the same key, which then may result in storing in the global cache dictionary a different cache than the one yielded by GetQueryCache.

Set as minor because the global cache dictionary is currently used only for statistics in NHibernate, most of them unimplemented. Now maybe some users may use it too, and have issues due to this.

[fredericDelaporte]

A bit moot to have this generated just for Thread.Sleep. But well, so be it.

[fredericDelaporte]

Initially it was nameof(RetrievedQueryCacheMatchesGloballyStoredOne), but this was invalid in the generated async code, due to the NewType generation option. Somewhat an async generator bug, but I do not think such a case is worth reporting.

[fredericDelaporte]

This fix does not avoid building many caches for the same region, but it ensures we store the same everywhere.
We should use https://stackoverflow.com/a/31637510/1178314 if we want to build only one cache per region. This will introduce additional locks, so it would have a performance impact.

[fredericDelaporte]

I now realize there is another bug here (already there without that PR): if the user asks for a query cache in a region used for an entity cache, a new cache will be built and will "replace" the entity cache in the global cache dictionary.

(Some cache provider may yield the same cache, having their own "cache of cache by region", especially for configured regions.)

I see many ways of fixing that other trouble:

• Adding some prefix to query region names before storing them in the global cache. Brittle, may breaks expectations on how to retrieve a query cache through GetSecondLevelCacheRegion.
• In the GetOrAdd value factory, try get the cache from the global cache and pass it for reuse to IQueryCacheFactory. Requires a new method on this interface and implies obsoleting the previous one.
• In the GetOrAdd value factory, try get the cache from the global cache and throw if already there, treating this as an error and asking the user for not re-using an entity region as a query region. This will require ensuring the value factory is executed only once per query cache with the Lazy<T> solution by example. This will be a breaking change for users already using the same region for query caches and entity caches.
• Leave this trouble in place. That global cache dictionary has no significant usages currently. (At least no one seems to have ever complained.) This troubles causes the cache yielded by GetSecondLevelCacheRegion to no more be the one held by persisters of entities in the affected region.

If choosing to fix that, it will not be enough to handles the "get query cache" case, because the same trouble exists with collection caches, which may override an entity cache too.

Maybe we should just trash away that global cache dictionary and what depends on it. (Obsoleting some statistics, GetSecondLevelCacheRegion and GetAllSecondLevelCacheRegions.)

[fredericDelaporte]

Looking more into cache initialization on factory build, there are even more issues, such as not respecting the cache concurrency if many entities use the same region, but with different concurrences. The first to be configured will set the concurrency, and the others will reuse it silently. But for collections, the trouble will not occurs, each will have their expected concurrency even if sharing the same region.

I will likely do another PR for cleaning the cache initialization logic. On Hibernate side, things have quite changed since 5.2 about that subject. They have moved most of the cache handling out of the session factory. Porting this should likely be done only for 6.0.

[fredericDelaporte]

Checking the history, this possibility of building then storing two different query caches for the same region is a regression from NH-3885, merged in 5.0 with #498. Previous to this, a lock was acquired, guaranteeing uniqueness of build.

I think we should go back to previous behavior, guaranteeing uniqueness of query cache built, by using a lazy.

[fredericDelaporte]

Each time I am looking into this file, it gets corrupted due to not being utf8 with that ç. Now re-encoded by the way.

[gliljas]

Facade, without the cedilla, is the most common (and correct) spelling anyway, so... ;)

[fredericDelaporte]

Now changed to c.

[fredericDelaporte]

I have completed the test for checking only one cache was now created.

[fredericDelaporte]

Missing async regen for a small change done on a previously merged PR. Unrelated but so small, let it get fixed here.

[hazzik]

I think this needs to go out of the lambda.

[fredericDelaporte]

The lazy lambda is the thing guaranteed to be executed only once. It looks to me as the natural place for putting access needing to be protected (here, guaranteeing we store the cache uniquely (only for all queries in this PR) built for the region).
Getting it out of the lambda would either mean re-storing it at each cache access, which looks inefficient, or will require some additional code like

// The factory may be run concurrently by threads trying to get the same region.
// But the GetOrAdd will yield the same lazy for all threads, so only one will
// initialize. https://stackoverflow.com/a/31637510/1178314
var lazyCache = queryCaches.GetOrAdd(
	cacheRegion,
	cr => new Lazy<IQueryCache>(
		() => settings.QueryCacheFactory.GetQueryCache(cr, updateTimestampsCache, settings, properties)));
if (lazyCache.IsValueCreated)
	return lazyCache.Value;
var cache = lazyCache.Value;
allCacheRegions[currentQueryCache.RegionName] = cache.Cache;
return cache;

This causes an overhead for all accesses to a cache by checking the IsValueCreated (maybe negligible, I do not know if this property is very lightweight or not), compared to the "fully initialized in lambda" one.
(And this one may also store multiple times the generated region cache, without creating an issue excepted slight inefficiency.)
Or using a boolean:

// The factory may be run concurrently by threads trying to get the same region.
// But the GetOrAdd will yield the same lazy for all threads, so only one will
// initialize. https://stackoverflow.com/a/31637510/1178314
var added = false;
var lazyCache = queryCaches.GetOrAdd(
	cacheRegion,
	cr => new Lazy<IQueryCache>(
		() => 
		{
			added = true;
			return settings.QueryCacheFactory.GetQueryCache(cr, updateTimestampsCache, settings, properties);
		}));
var cache = lazyCache.Value;
if (added)
	allCacheRegions[currentQueryCache.RegionName] = cache.Cache;
return cache;

So now the overhead is very small, just a boolean check. But there we capture a local variable into the lambda for mutating it, which is far more debatable than just doing the whole initialization in it.

Moreover, both propositions above are highly sensitive to code lines order: the IsValueCreated must of course be checked before accessing Value, while the added boolean must be checked after.

So those other implementations look to me as only having disadvantages.

Maybe would you just want this lambda to be converted as full blown function?

	// The factory may be run concurrently by threads trying to get the same region.
	// 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 => new Lazy<IQueryCache>(
			() => CreateQueryCache(cr))).Value;
}

private IQueryCache CreateQueryCache(string region)
{
	var cache = settings.QueryCacheFactory.GetQueryCache(cr, updateTimestampsCache, settings, properties);
	allCacheRegions[currentQueryCache.RegionName] = cache.Cache;
	return cache;
}

Or something else? But I do not see what.

[fredericDelaporte]

Rebased.