Skip to content

Commit eeaf857

Browse files
committed
use partitioned lock to avoid minor race condition on "add" vs "remove" paths
1 parent 5c99d4d commit eeaf857

File tree

6 files changed

+88
-9
lines changed

6 files changed

+88
-9
lines changed

src/Caching/Hybrid/src/Internal/DefaultHybridCache.Stampede.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public bool GetOrCreateStampedeState<TState, T>(string key, HybridCacheEntryFlag
4343
// hmm; failed to add - there's concurrent activity on the same key; we're now
4444
// in very rare race condition territory; go ahead and take a lock while we
4545
// collect our thoughts
46-
lock (_currentOperations)
46+
lock (GetPartitionedSyncLock(in stampedeKey)) // see notes in SyncLock.cs
4747
{
4848
// check again while we hold the lock
4949
if (TryJoinExistingSession(this, stampedeKey, out existing))

src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeKey.cs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public StampedeKey(string key, HybridCacheEntryFlags flags)
2626
_key = key;
2727
_flags = flags;
2828
#if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1_OR_GREATER
29-
_hashCode = HashCode.Combine(key, flags);
29+
_hashCode = System.HashCode.Combine(key, flags);
3030
#else
3131
_hashCode = key.GetHashCode() ^ (int)flags;
3232
#endif
@@ -35,6 +35,10 @@ public StampedeKey(string key, HybridCacheEntryFlags flags)
3535
public string Key => _key;
3636
public HybridCacheEntryFlags Flags => _flags;
3737

38+
// allow direct access to the pre-computed hash-code, semantically emphasizing that
39+
// this is a constant-time operation against a known value
40+
internal int HashCode => _hashCode;
41+
3842
public bool Equals(StampedeKey other) => _flags == other._flags & _key == other._key;
3943

4044
public override bool Equals([NotNullWhen(true)] object? obj)

src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeState.cs

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,18 @@ internal abstract class StampedeState
2222
private readonly CancellationTokenSource? _sharedCancellation;
2323
internal readonly CancellationToken SharedToken; // this might have a value even when _sharedCancellation is null
2424

25-
public StampedeKey Key { get; }
25+
// we expose the key as a by-ref readonly; this minimizes the stack work involved in passing the key around
26+
// (both in terms of width and copy-semantics)
27+
private readonly StampedeKey _key;
28+
public ref readonly StampedeKey Key => ref _key;
2629

2730
/// <summary>
2831
/// Create a stamped token optionally with shared cancellation support
2932
/// </summary>
3033
protected StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBeCanceled)
3134
{
3235
_cache = cache;
33-
Key = key;
36+
_key = key;
3437
if (canBeCanceled)
3538
{
3639
// if the first (or any) caller can't be cancelled; we'll never get to zero; no point tracking
@@ -50,7 +53,7 @@ protected StampedeState(DefaultHybridCache cache, in StampedeKey key, bool canBe
5053
protected StampedeState(DefaultHybridCache cache, in StampedeKey key, CancellationToken token)
5154
{
5255
_cache = cache;
53-
Key = key;
56+
_key = key;
5457
SharedToken = token;
5558
}
5659

@@ -103,5 +106,11 @@ public bool TryAddCaller() // essentially just interlocked-increment, but with a
103106
}
104107
}
105108

106-
private void RemoveStampedeState(StampedeKey key) => _currentOperations.TryRemove(key, out _);
109+
private void RemoveStampedeState(in StampedeKey key)
110+
{
111+
lock (GetPartitionedSyncLock(in key)) // see notes in SyncLock.cs
112+
{
113+
_currentOperations.TryRemove(key, out _);
114+
}
115+
}
107116
}

src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeStateT.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ private void SetException(Exception ex)
134134
{
135135
if (_result is not null)
136136
{
137-
Cache.RemoveStampedeState(Key);
137+
Cache.RemoveStampedeState(in Key);
138138
_result.TrySetException(ex);
139139
}
140140
}
@@ -148,7 +148,7 @@ private void SetResult(CacheItem<T> value)
148148

149149
if (_result is not null)
150150
{
151-
Cache.RemoveStampedeState(Key);
151+
Cache.RemoveStampedeState(in Key);
152152
_result.TrySetResult(value);
153153
}
154154
}
@@ -158,7 +158,7 @@ private void SetDefaultResult()
158158
// note we don't store this dummy result in L1 or L2
159159
if (_result is not null)
160160
{
161-
Cache.RemoveStampedeState(Key);
161+
Cache.RemoveStampedeState(in Key);
162162
_result.TrySetResult(ImmutableCacheItem<T>.Default);
163163
}
164164
}
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
namespace Microsoft.Extensions.Caching.Hybrid.Internal;
5+
6+
partial class DefaultHybridCache
7+
{
8+
// HybridCache's stampede protection requires some level of synchronization to avoid unnecessary runs
9+
// of the underlying data fetch; this is *minimized* by the use of double-checked locking and
10+
// interlocked join (adding a new request to an existing execution), but: that would leave a race
11+
// condition where the *remove* step of the stampede would be in a race with the *add new* step; the
12+
// *add new* step is inside a lock, but we need to *remove* step to share that lock, to avoid
13+
// the race. We deal with that by taking the same lock during remove, but *that* means we're locking
14+
// on all executions.
15+
//
16+
// To minimize lock contention, we will therefore use partitioning of the lock-token, by using the
17+
// low 3 bits of the hash-code (which we calculate eagerly only one, so: already know). This gives
18+
// us a fast way to split contention 8, almost an order-of-magnitude, which is sufficient. We *could*
19+
// use an array for this, but: for directness, let's inline it instead (avoiding bounds-checks,
20+
// an extra layer of dereferencing, and the allocation; I will acknowledge these are miniscule, but:
21+
// it costs us nothing to do)
22+
23+
private readonly object _syncLock0 = new(), _syncLock1 = new(), _syncLock2 = new(), _syncLock3 = new(),
24+
_syncLock4 = new(), _syncLock5 = new(), _syncLock6 = new(), _syncLock7 = new();
25+
26+
internal object GetPartitionedSyncLock(in StampedeKey key)
27+
=> (key.HashCode & 0b111) switch // generate 8 partitions using the low 3 bits
28+
{
29+
0 => _syncLock0, 1 => _syncLock1,
30+
2 => _syncLock2, 3 => _syncLock3,
31+
4 => _syncLock4, 5 => _syncLock5,
32+
6 => _syncLock6, _ => _syncLock7,
33+
};
34+
}

src/Caching/Hybrid/test/StampedeTests.cs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,38 @@ public async Task MutableTypesNeverShareFinalTask(bool withCancelation)
365365
Assert.NotSame(x, y);
366366
}
367367

368+
[Fact]
369+
public void ValidatePartitioning()
370+
{
371+
// we just want to validate that key-level partitioning is
372+
// happening to some degree, i.e. it isn't fundamentally broken
373+
using var scope = GetDefaultCache(out var cache);
374+
Dictionary<object, int> counts = [];
375+
for(int i = 0; i < 1024; i++)
376+
{
377+
var key = new DefaultHybridCache.StampedeKey(Guid.NewGuid().ToString(), default);
378+
var obj = cache.GetPartitionedSyncLock(in key);
379+
if (!counts.TryGetValue(obj, out var count))
380+
{
381+
count = 0;
382+
}
383+
counts[obj] = count + 1;
384+
}
385+
386+
// We just want to prove that we got 8 non-empty partitions.
387+
// This is *technically* non-deterministic, but: we'd
388+
// need to be having a very bad day for the math gods
389+
// to conspire against us that badly - if this test
390+
// starts failing, maybe buy a lottery ticket?
391+
Assert.Equal(8, counts.Count);
392+
foreach (var pair in counts)
393+
{
394+
// the *median* should be 128 here; let's
395+
// not be aggressive about it, though
396+
Assert.True(pair.Value > 16);
397+
}
398+
}
399+
368400
class Mutable(Guid value)
369401
{
370402
public Guid Value => value;

0 commit comments

Comments
 (0)