Skip to content

Commit d251195

Browse files
TurnerjTurnerj
authored
Merge pull request #193 from rgueldenpfennig/nullreferenceexception-on-concurrent-access
 NullReferenceException on blocked task when original task faulted
2 parents 2e3ac6d + 5730f9d commit d251195

2 files changed

Lines changed: 118 additions & 20 deletions

File tree

‎src/CacheTower/CacheStack.cs‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ public async ValueTask FlushAsync()
7070
public async ValueTask CleanupAsync()
7171
{
7272
ThrowIfDisposed();
73-
73+
7474
for (int i = 0, l = CacheLayers.Length; i < l; i++)
7575
{
7676
var layer = CacheLayers[i];
@@ -315,16 +315,16 @@ private async ValueTask BackPopulateCacheAsync<T>(int fromIndexExclusive, string
315315
return refreshedEntry;
316316
}, settings);
317317
}
318-
catch
318+
catch (Exception e)
319319
{
320-
UnlockWaitingTasks(cacheKey, null!);
320+
UnlockWaitingTasks(cacheKey, e);
321321
throw;
322322
}
323323
}
324324
else if (noExistingValueAvailable)
325325
{
326326
TaskCompletionSource<CacheEntry> completionSource;
327-
327+
328328
lock (WaitingKeyRefresh)
329329
{
330330
if (!WaitingKeyRefresh.TryGetValue(cacheKey, out completionSource!) || completionSource == null)
@@ -362,6 +362,18 @@ private void UnlockWaitingTasks(string cacheKey, CacheEntry cacheEntry)
362362
}
363363
}
364364

365+
private void UnlockWaitingTasks(string cacheKey, Exception exception)
366+
{
367+
lock (WaitingKeyRefresh)
368+
{
369+
if (WaitingKeyRefresh.TryGetValue(cacheKey, out var completionSource))
370+
{
371+
WaitingKeyRefresh.Remove(cacheKey);
372+
completionSource?.SetException(exception);
373+
}
374+
}
375+
}
376+
365377
/// <summary>
366378
/// Disposes the current instance of <see cref="CacheStack"/> and all associated layers and extensions.
367379
/// </summary>

‎tests/CacheTower.Tests/CacheStackTests.cs‎

Lines changed: 102 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
1-
using CacheTower.Providers.Memory;
2-
using System;
1+
using System;
32
using System.Collections.Generic;
3+
using System.Linq;
4+
using System.Threading;
45
using System.Threading.Tasks;
6+
using CacheTower.Providers.Memory;
57
using Microsoft.VisualStudio.TestTools.UnitTesting;
68
using Moq;
7-
using System.Threading;
89

910
namespace CacheTower.Tests
1011
{
@@ -32,7 +33,7 @@ public async Task Cleanup_CleansAllTheLayers()
3233
{
3334
var layer1 = new MemoryCacheLayer();
3435
var layer2 = new MemoryCacheLayer();
35-
36+
3637
await using var cacheStack = new CacheStack(new[] { layer1, layer2 }, Array.Empty<ICacheExtension>());
3738

3839
var cacheEntry = new CacheEntry<int>(42, DateTime.UtcNow.AddDays(-1));
@@ -50,7 +51,8 @@ public async Task Cleanup_CleansAllTheLayers()
5051
public async Task Cleanup_ThrowsOnUseAfterDisposal()
5152
{
5253
var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, null);
53-
await using (cacheStack) { }
54+
await using (cacheStack)
55+
{ }
5456

5557
await cacheStack.CleanupAsync();
5658
}
@@ -82,7 +84,7 @@ public async Task Evict_EvictsAllTheLayers()
8284
public async Task Evict_TriggersCacheChangeExtension()
8385
{
8486
var mockExtension = new Mock<ICacheChangeExtension>();
85-
await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, new [] { mockExtension.Object });
87+
await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, new[] { mockExtension.Object });
8688
var cacheEntry = await cacheStack.SetAsync("Evict_TriggerCacheChangeExtension", 42, TimeSpan.FromDays(1));
8789

8890
await cacheStack.EvictAsync("Evict_TriggerCacheChangeExtension");
@@ -93,7 +95,8 @@ public async Task Evict_TriggersCacheChangeExtension()
9395
public async Task Evict_ThrowsOnUseAfterDisposal()
9496
{
9597
var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, null);
96-
await using (cacheStack) { }
98+
await using (cacheStack)
99+
{ }
97100

98101
await cacheStack.EvictAsync("KeyDoesntMatter");
99102
}
@@ -146,7 +149,8 @@ public async Task Get_ThrowsOnNullKey()
146149
public async Task Get_ThrowsOnUseAfterDisposal()
147150
{
148151
var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, null);
149-
await using (cacheStack) { }
152+
await using (cacheStack)
153+
{ }
150154

151155
await cacheStack.GetAsync<int>("KeyDoesntMatter");
152156
}
@@ -168,15 +172,17 @@ public async Task Set_ThrowsOnNullCacheEntry()
168172
public async Task Set_ThrowsOnUseAfterDisposal()
169173
{
170174
var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, null);
171-
await using (cacheStack) { }
175+
await using (cacheStack)
176+
{ }
172177

173178
await cacheStack.SetAsync("KeyDoesntMatter", 1, TimeSpan.FromDays(1));
174179
}
175180
[TestMethod, ExpectedException(typeof(ObjectDisposedException))]
176181
public async Task Set_ThrowsOnUseAfterDisposal_CacheEntry()
177182
{
178183
var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, null);
179-
await using (cacheStack) { }
184+
await using (cacheStack)
185+
{ }
180186

181187
await cacheStack.SetAsync("KeyDoesntMatter", new CacheEntry<int>(1, TimeSpan.FromDays(1)));
182188
}
@@ -218,10 +224,10 @@ public async Task GetOrSet_ThrowsOnNullGetter()
218224
public async Task GetOrSet_CacheMiss()
219225
{
220226
await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, Array.Empty<ICacheExtension>());
221-
var result = await cacheStack.GetOrSetAsync<int>("GetOrSet_CacheMiss", (oldValue) =>
222-
{
223-
return Task.FromResult(5);
224-
}, new CacheSettings(TimeSpan.FromDays(1)));
227+
var result = await cacheStack.GetOrSetAsync<int>("GetOrSet_CacheMiss", (oldValue) =>
228+
{
229+
return Task.FromResult(5);
230+
}, new CacheSettings(TimeSpan.FromDays(1)));
225231

226232
Assert.AreEqual(5, result);
227233
}
@@ -321,11 +327,91 @@ await cacheStack.GetOrSetAsync<int>(
321327

322328
Assert.AreEqual(1, getterCallCount);
323329
}
330+
[TestMethod]
331+
public async Task GetOrSet_ConcurrentAccess_OnException()
332+
{
333+
await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, Array.Empty<ICacheExtension>());
334+
335+
Internal.DateTimeProvider.UpdateTime();
336+
var getterCallCount = 0;
337+
338+
var expectedException = new InvalidOperationException("Some exception");
339+
async Task act()
340+
{
341+
await cacheStack.GetOrSetAsync<int>(
342+
"GetOrSet_ConcurrentAccess_OnException",
343+
async _ =>
344+
{
345+
Interlocked.Increment(ref getterCallCount);
346+
await Task.Delay(100);
347+
throw expectedException;
348+
},
349+
new CacheSettings(TimeSpan.FromDays(2), TimeSpan.Zero)
350+
);
351+
}
352+
353+
var tasks = Enumerable.Range(1, 2)
354+
.Select(i => act())
355+
.ToArray();
356+
357+
try
358+
{
359+
await Task.WhenAll(tasks);
360+
}
361+
catch (Exception e)
362+
{
363+
Assert.IsInstanceOfType(e, expectedException.GetType());
364+
}
365+
366+
Assert.AreEqual(1, getterCallCount);
367+
foreach (var task in tasks)
368+
{
369+
Assert.AreSame(expectedException, task.Exception.InnerException);
370+
}
371+
}
372+
[TestMethod]
373+
[DataRow(null)]
374+
[DataRow(42)]
375+
public async Task GetOrSet_ConcurrentAccess_SameResultForAllTasks(int? expectedResult)
376+
{
377+
await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, Array.Empty<ICacheExtension>());
378+
379+
Internal.DateTimeProvider.UpdateTime();
380+
var getterCallCount = 0;
381+
382+
async Task<int?> act()
383+
{
384+
return await cacheStack.GetOrSetAsync<int?>(
385+
"GetOrSet_ConcurrentAccess_SameResultForAllTasks",
386+
async _ =>
387+
{
388+
Interlocked.Increment(ref getterCallCount);
389+
await Task.Delay(100);
390+
return expectedResult;
391+
},
392+
new CacheSettings(TimeSpan.FromDays(2), TimeSpan.Zero)
393+
);
394+
}
395+
396+
var tasks = Enumerable.Range(1, 4)
397+
.Select(i => act())
398+
.ToArray();
399+
400+
var whenAll = Task.WhenAll(tasks);
401+
await Task.WhenAny(whenAll, Task.Delay(TimeSpan.FromSeconds(1)));
402+
403+
Assert.AreEqual(1, getterCallCount);
404+
foreach (var task in tasks)
405+
{
406+
Assert.AreEqual(expectedResult, task.Result);
407+
}
408+
}
324409
[TestMethod, ExpectedException(typeof(ObjectDisposedException))]
325410
public async Task GetOrSet_ThrowsOnUseAfterDisposal()
326411
{
327412
var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, null);
328-
await using (cacheStack) { }
413+
await using (cacheStack)
414+
{ }
329415

330416
await cacheStack.GetOrSetAsync<int>("KeyDoesntMatter", (old) => Task.FromResult(1), new CacheSettings(TimeSpan.FromDays(1)));
331417
}
@@ -335,7 +421,7 @@ public async Task GetOrSet_WaitingForRefresh()
335421
await using var cacheStack = new CacheStack(new[] { new MemoryCacheLayer() }, null);
336422
var gettingLockSource = new TaskCompletionSource<bool>();
337423
var continueRefreshSource = new TaskCompletionSource<bool>();
338-
424+
339425
var cacheGetThatLocksOnRefresh = cacheStack.GetOrSetAsync<int>("GetOrSet_WaitingForRefresh", async (old) =>
340426
{
341427
gettingLockSource.SetResult(true);

0 commit comments

Comments
 (0)