Skip to content

Commit 9e7ad12

Browse files
fredericDelaportefredericDelaporte
authored
Fix GetQueryCache storing two different caches. (#1476)
And fix GetQueryCache building two different caches for same region.
1 parent dce4d02 commit 9e7ad12

File tree

4 files changed

+241
-18
lines changed

4 files changed

+241
-18
lines changed

‎src/NHibernate.Test/Async/CacheTest/GetQueryCacheFixture.cs

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
//------------------------------------------------------------------------------
2+
// <auto-generated>
3+
// This code was generated by AsyncGenerator.
4+
//
5+
// Changes to this file may cause incorrect behavior and will be lost if
6+
// the code is regenerated.
7+
// </auto-generated>
8+
//------------------------------------------------------------------------------
9+
10+
11+
using System;
12+
using System.Collections;
13+
using System.Collections.Concurrent;
14+
using System.Collections.Generic;
15+
using System.Threading;
16+
using NHibernate.Cache;
17+
using NHibernate.Cfg;
18+
using NUnit.Framework;
19+
using Environment = NHibernate.Cfg.Environment;
20+
21+
namespace NHibernate.Test.CacheTest
22+
{
23+
using System.Threading.Tasks;
24+
[TestFixture]
25+
public class GetQueryCacheFixtureAsync : TestCase
26+
{
27+
protected override IList Mappings => new[] { "Simple.hbm.xml" };
28+
29+
protected override void Configure(Configuration configuration)
30+
{
31+
configuration.SetProperty(Environment.UseQueryCache, "true");
32+
configuration.SetProperty(Environment.CacheProvider, typeof(LockedCacheProvider).AssemblyQualifiedName);
33+
}
34+
35+
[Test]
36+
public async Task RetrievedQueryCacheMatchesGloballyStoredOneAsync()
37+
{
38+
var region = "RetrievedQueryCacheMatchesGloballyStoredOne";
39+
LockedCache.Semaphore = new SemaphoreSlim(0);
40+
LockedCache.CreationCount = 0;
41+
try
42+
{
43+
var failures = new ConcurrentBag<Exception>();
44+
var thread1 = new Thread(
45+
() =>
46+
{
47+
try
48+
{
49+
Sfi.GetQueryCache(region);
50+
}
51+
catch (Exception e)
52+
{
53+
failures.Add(e);
54+
}
55+
});
56+
var thread2 = new Thread(
57+
() =>
58+
{
59+
try
60+
{
61+
Sfi.GetQueryCache(region);
62+
}
63+
catch (Exception e)
64+
{
65+
failures.Add(e);
66+
}
67+
});
68+
thread1.Start();
69+
thread2.Start();
70+
71+
// Give some time to threads for reaching the wait, having all of them ready to do most of their job concurrently.
72+
await (Task.Delay(100));
73+
// Let only one finish its job, it should be the one being stored in query cache dictionary.
74+
LockedCache.Semaphore.Release(1);
75+
// Give some time to released thread to finish its job.
76+
await (Task.Delay(100));
77+
// Release other thread
78+
LockedCache.Semaphore.Release(10);
79+
80+
thread1.Join();
81+
thread2.Join();
82+
Assert.That(failures, Is.Empty, $"{failures.Count} thread(s) failed.");
83+
}
84+
finally
85+
{
86+
LockedCache.Semaphore.Dispose();
87+
LockedCache.Semaphore = null;
88+
}
89+
90+
var queryCache = Sfi.GetQueryCache(region).Cache;
91+
var globalCache = Sfi.GetSecondLevelCacheRegion(region);
92+
Assert.That(globalCache, Is.SameAs(queryCache));
93+
Assert.That(LockedCache.CreationCount, Is.EqualTo(1));
94+
}
95+
}
96+
}

‎src/NHibernate.Test/CacheTest/GetQueryCacheFixture.cs

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
using System;
2+
using System.Collections;
3+
using System.Collections.Concurrent;
4+
using System.Collections.Generic;
5+
using System.Threading;
6+
using NHibernate.Cache;
7+
using NHibernate.Cfg;
8+
using NUnit.Framework;
9+
using Environment = NHibernate.Cfg.Environment;
10+
11+
namespace NHibernate.Test.CacheTest
12+
{
13+
[TestFixture]
14+
public class GetQueryCacheFixture : TestCase
15+
{
16+
protected override IList Mappings => new[] { "Simple.hbm.xml" };
17+
18+
protected override void Configure(Configuration configuration)
19+
{
20+
configuration.SetProperty(Environment.UseQueryCache, "true");
21+
configuration.SetProperty(Environment.CacheProvider, typeof(LockedCacheProvider).AssemblyQualifiedName);
22+
}
23+
24+
[Test]
25+
public void RetrievedQueryCacheMatchesGloballyStoredOne()
26+
{
27+
var region = "RetrievedQueryCacheMatchesGloballyStoredOne";
28+
LockedCache.Semaphore = new SemaphoreSlim(0);
29+
LockedCache.CreationCount = 0;
30+
try
31+
{
32+
var failures = new ConcurrentBag<Exception>();
33+
var thread1 = new Thread(
34+
() =>
35+
{
36+
try
37+
{
38+
Sfi.GetQueryCache(region);
39+
}
40+
catch (Exception e)
41+
{
42+
failures.Add(e);
43+
}
44+
});
45+
var thread2 = new Thread(
46+
() =>
47+
{
48+
try
49+
{
50+
Sfi.GetQueryCache(region);
51+
}
52+
catch (Exception e)
53+
{
54+
failures.Add(e);
55+
}
56+
});
57+
thread1.Start();
58+
thread2.Start();
59+
60+
// Give some time to threads for reaching the wait, having all of them ready to do most of their job concurrently.
61+
Thread.Sleep(100);
62+
// Let only one finish its job, it should be the one being stored in query cache dictionary.
63+
LockedCache.Semaphore.Release(1);
64+
// Give some time to released thread to finish its job.
65+
Thread.Sleep(100);
66+
// Release other thread
67+
LockedCache.Semaphore.Release(10);
68+
69+
thread1.Join();
70+
thread2.Join();
71+
Assert.That(failures, Is.Empty, $"{failures.Count} thread(s) failed.");
72+
}
73+
finally
74+
{
75+
LockedCache.Semaphore.Dispose();
76+
LockedCache.Semaphore = null;
77+
}
78+
79+
var queryCache = Sfi.GetQueryCache(region).Cache;
80+
var globalCache = Sfi.GetSecondLevelCacheRegion(region);
81+
Assert.That(globalCache, Is.SameAs(queryCache));
82+
Assert.That(LockedCache.CreationCount, Is.EqualTo(1));
83+
}
84+
}
85+
86+
public class LockedCache : HashtableCache
87+
{
88+
public static SemaphoreSlim Semaphore { get; set; }
89+
90+
private static int _creationCount;
91+
92+
public static int CreationCount
93+
{
94+
get => _creationCount;
95+
set => _creationCount = value;
96+
}
97+
98+
public LockedCache(string regionName) : base(regionName)
99+
{
100+
Semaphore?.Wait();
101+
Interlocked.Increment(ref _creationCount);
102+
}
103+
}
104+
105+
public class LockedCacheProvider : ICacheProvider
106+
{
107+
public ICache BuildCache(string regionName, IDictionary<string, string> properties)
108+
{
109+
return new LockedCache(regionName);
110+
}
111+
112+
public long NextTimestamp()
113+
{
114+
return Timestamper.Next();
115+
}
116+
117+
public void Start(IDictionary<string, string> properties)
118+
{
119+
}
120+
121+
public void Stop()
122+
{
123+
}
124+
}
125+
}

‎src/NHibernate/Async/Impl/SessionFactoryImpl.cs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,9 @@ public sealed partial class SessionFactoryImpl : ISessionFactoryImplementor, IOb
8787
{
8888
queryCache.Destroy();
8989

90-
foreach (IQueryCache cache in queryCaches.Values)
90+
foreach (var cache in queryCaches.Values)
9191
{
92-
cache.Destroy();
92+
cache.Value.Destroy();
9393
}
9494

9595
updateTimestampsCache.Destroy();
@@ -297,10 +297,9 @@ public sealed partial class SessionFactoryImpl : ISessionFactoryImplementor, IOb
297297
{
298298
if (settings.IsQueryCacheEnabled)
299299
{
300-
IQueryCache currentQueryCache;
301-
if (queryCaches.TryGetValue(cacheRegion, out currentQueryCache))
300+
if (queryCaches.TryGetValue(cacheRegion, out var currentQueryCache))
302301
{
303-
return currentQueryCache.ClearAsync(cancellationToken);
302+
return currentQueryCache.Value.ClearAsync(cancellationToken);
304303
}
305304
}
306305
}

‎src/NHibernate/Impl/SessionFactoryImpl.cs

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ public void HandleEntityNotFound(string entityName, object id)
149149
private readonly IQueryCache queryCache;
150150

151151
[NonSerialized]
152-
private readonly ConcurrentDictionary<string, IQueryCache> queryCaches;
152+
private readonly ConcurrentDictionary<string, Lazy<IQueryCache>> queryCaches;
153153
[NonSerialized]
154154
private readonly SchemaExport schemaExport;
155155
[NonSerialized]
@@ -381,7 +381,7 @@ public SessionFactoryImpl(Configuration cfg, IMapping mapping, Settings settings
381381
{
382382
updateTimestampsCache = new UpdateTimestampsCache(settings, properties);
383383
queryCache = settings.QueryCacheFactory.GetQueryCache(null, updateTimestampsCache, settings, properties);
384-
queryCaches = new ConcurrentDictionary<string, IQueryCache>();
384+
queryCaches = new ConcurrentDictionary<string, Lazy<IQueryCache>>();
385385
}
386386
else
387387
{
@@ -847,9 +847,9 @@ public void Close()
847847
{
848848
queryCache.Destroy();
849849

850-
foreach (IQueryCache cache in queryCaches.Values)
850+
foreach (var cache in queryCaches.Values)
851851
{
852-
cache.Destroy();
852+
cache.Value.Destroy();
853853
}
854854

855855
updateTimestampsCache.Destroy();
@@ -1029,14 +1029,18 @@ public IQueryCache GetQueryCache(string cacheRegion)
10291029
return null;
10301030
}
10311031

1032+
// The factory may be run concurrently by threads trying to get the same region.
1033+
// But the GetOrAdd will yield the same lazy for all threads, so only one will
1034+
// initialize. https://stackoverflow.com/a/31637510/1178314
10321035
return queryCaches.GetOrAdd(
10331036
cacheRegion,
1034-
cr =>
1035-
{
1036-
IQueryCache currentQueryCache = settings.QueryCacheFactory.GetQueryCache(cacheRegion, updateTimestampsCache, settings, properties);
1037-
allCacheRegions[currentQueryCache.RegionName] = currentQueryCache.Cache;
1038-
return currentQueryCache;
1039-
});
1037+
cr => new Lazy<IQueryCache>(
1038+
() =>
1039+
{
1040+
var currentQueryCache = settings.QueryCacheFactory.GetQueryCache(cr, updateTimestampsCache, settings, properties);
1041+
allCacheRegions[currentQueryCache.RegionName] = currentQueryCache.Cache;
1042+
return currentQueryCache;
1043+
})).Value;
10401044
}
10411045

10421046
public void EvictQueries()
@@ -1062,10 +1066,9 @@ public void EvictQueries(string cacheRegion)
10621066
{
10631067
if (settings.IsQueryCacheEnabled)
10641068
{
1065-
IQueryCache currentQueryCache;
1066-
if (queryCaches.TryGetValue(cacheRegion, out currentQueryCache))
1069+
if (queryCaches.TryGetValue(cacheRegion, out var currentQueryCache))
10671070
{
1068-
currentQueryCache.Clear();
1071+
currentQueryCache.Value.Clear();
10691072
}
10701073
}
10711074
}

0 commit comments

Comments
 (0)