-
Notifications
You must be signed in to change notification settings - Fork 934
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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() | ||
{ | ||
var region = "RetrievedQueryCacheMatchesGloballyStoredOne"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initially it was |
||
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)); | ||
} | ||
} | ||
} |
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() | ||
{ | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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] | ||
|
@@ -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 | ||
{ | ||
|
@@ -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,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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this needs to go out of the lambda. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). // 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 // 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 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() | ||
|
@@ -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(); | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
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.