Skip to content

Fix GetQueryCache storing two different caches. #1476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 11, 2018
Merged

Fix GetQueryCache storing two different caches. #1476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e13b461
Fix GetQueryCache storing two different caches.
fredericDelaporte Dec 7, 2017
ea4b2d2
Fix GetQueryCache building two different caches for same region.
fredericDelaporte Dec 8, 2017
f23bde1
Merge branch 'master' into GetQueryCacheConcurrency
fredericDelaporte Mar 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 96 additions & 0 deletions src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
//------------------------------------------------------------------------------
// <auto-generated>
// This code was generated by AsyncGenerator.
//
// Changes to this file may cause incorrect behavior and will be lost if
// the code is regenerated.
// </auto-generated>
//------------------------------------------------------------------------------


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()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
var region = "RetrievedQueryCacheMatchesGloballyStoredOne";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

LockedCache.Semaphore = new SemaphoreSlim(0);
LockedCache.CreationCount = 0;
try
{
var failures = new ConcurrentBag<Exception>();
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));
Assert.That(LockedCache.CreationCount, Is.EqualTo(1));
}
}
}
125 changes: 125 additions & 0 deletions src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
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);
LockedCache.CreationCount = 0;
try
{
var failures = new ConcurrentBag<Exception>();
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));
Assert.That(LockedCache.CreationCount, Is.EqualTo(1));
}
}

public class LockedCache : HashtableCache
{
public static SemaphoreSlim Semaphore { get; set; }

private static int _creationCount;

public static int CreationCount
{
get => _creationCount;
set => _creationCount = value;
}

public LockedCache(string regionName) : base(regionName)
{
Semaphore?.Wait();
Interlocked.Increment(ref _creationCount);
}
}

public class LockedCacheProvider : ICacheProvider
{
public ICache BuildCache(string regionName, IDictionary<string, string> properties)
{
return new LockedCache(regionName);
}

public long NextTimestamp()
{
return Timestamper.Next();
}

public void Start(IDictionary<string, string> properties)
{
}

public void Stop()
{
}
}
}
9 changes: 4 additions & 5 deletions src/NHibernate/Async/Impl/SessionFactoryImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}
}
Expand Down
29 changes: 16 additions & 13 deletions src/NHibernate/Impl/SessionFactoryImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public void HandleEntityNotFound(string entityName, object id)
private readonly IQueryCache queryCache;

[NonSerialized]
private readonly ConcurrentDictionary<string, IQueryCache> queryCaches;
private readonly ConcurrentDictionary<string, Lazy<IQueryCache>> queryCaches;
[NonSerialized]
private readonly SchemaExport schemaExport;
[NonSerialized]
Expand Down Expand Up @@ -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<string, IQueryCache>();
queryCaches = new ConcurrentDictionary<string, Lazy<IQueryCache>>();
}
else
{
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -1029,14 +1029,18 @@ public IQueryCache GetQueryCache(string cacheRegion)
return null;
}

// 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 =>
{
IQueryCache currentQueryCache = settings.QueryCacheFactory.GetQueryCache(cacheRegion, updateTimestampsCache, settings, properties);
allCacheRegions[currentQueryCache.RegionName] = currentQueryCache.Cache;
return currentQueryCache;
});
cr => new Lazy<IQueryCache>(
() =>
{
var currentQueryCache = settings.QueryCacheFactory.GetQueryCache(cr, updateTimestampsCache, settings, properties);
allCacheRegions[currentQueryCache.RegionName] = currentQueryCache.Cache;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to go out of the lambda.

Copy link
Member Author

@fredericDelaporte fredericDelaporte Dec 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return currentQueryCache;
})).Value;
}

public void EvictQueries()
Expand All @@ -1062,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();
}
}
}
Expand Down