HybridCache - minor post-PR fixes

[mgravell]

This is supplementary post-PR (it was auto-merged) tweaks requested in #55147, mostly for @amcasey - additional comments, etc

---

Additional notes on 7f8fbc6 - this removes any special treatment of IDisposable; this is because trying to consider it causes semantic problems; the caller can't know the lifetime, so if we did this, we would have a scenario where:

var obj = cache.GetOrCreateAsync(...);
// ... other stuff
obj.DoSomething(); // boom ObjectDisposedException

because during the "... other stuff", the shared instance got evicted from cache and disposed; this is unpredictable and not a supportable scenario. The correct behaviours are:

1. the caller gets a new instance and disposes per-call, i.e. using var obj = cache.GetOrCreateAsync(...), or
2. it doesn't get disposed at all, using GC and finalizers if necessary

(the latter being less "correct" than the former)

Both of these are what we already get from simply not considering IDisposable at all; 1. is what we get if the type is considered mutable, and 2. is what we get if it is considered immutable; so: it is already in the consumer's hands.

In reality, it very much isn't worth worrying to much about IDisposable here; the intended use-case of cache is for transient state data, typically POCOs - not connected services etc (we're not the DI layer). IMO we should not overly distort any part of the design worrying about a pathological and hypothetical use-case that is probably best avoided with the words: "don't do that".

---

additional notes on 223f1bd - this avoids having to deserialize inside the sync-lock (which is horrible), but to do that we need a single unified ref-count rather than the two separate ref-counts we had previously (one for the stampede state, one for the cache item), so that we can speculatively reserve our value without having to deserialize it; this actually simplifies the logic, so is desirable in its own right.

[AnthonyLloyd]

Hi, I may easily be missing something but don't you have to check the local cache again after you find you are the stampede that needs to do the work? There's a race between the dictionaries where someone was adding the value and removed themselves from the _currentOperations just before you got there. Or maybe this is very unlikely? I noticed because I implemented something similar recently (without the cool stampede name).

[mgravell]

@AnthonyLloyd can you outline the scenario you're thinking of, that isn't guarded by the interlocked join and the double-checked locking? The only scenario I can see there is when an in-flight operations is on the cusp of terminating, which means the try-join inside the double-checked lock fails. If that is the scenario you're thinking of, we could potentially take the lock for the final decrement+remove. That is rare but theoretically non-zero - I could try to measure impact of adding a lock here (probably negligible because we try gard to minimize the locks on the competing path). Or are you thinking of another scenario?

[AnthonyLloyd]

@mgravell I think it's what you describe:
Thread 1: Has got all the way to just before Cache.SetL1.
Thread 2: Sees nothing in _localCache and is just about to call GetOrCreateStampedeState.
Thread 1: Completes Cache.SetL1 and removes itself from _currentOperations before Thread 2 accesses _currentOperations.
This would give a completed value in _localCache with Thread 2 working to get a duplicate.

[mgravell]

OK. Probably won't be ready in time for preview 4, but I'll see what the impact of preventing that scenario is.

[mgravell]

Fun fact: in an incomplete commit earlier, I actually had a partitioned sync-lock grid to avoid any remaining contention on the lock - I ditched that when the only use was the double-check, which is itself rare. If we are going to lock on the remove path, it is probably worth resurrecting that; just a simple partition just using the low 3 bits of the key's hashcode.

[mgravell]

@AnthonyLloyd eeaf857

[AnthonyLloyd]

@mgravell Does it not still have the problem that mutliple threads could come in at the same time, see a local cache miss and then all queue up on the lock and repeat the call? Is the idea that the L2 call can be repeated? And if there is no remote L2 you use L2 = L1 and hence check the local cache again? Sorry I've not studied the code fully.

Ah I see you are now checking L1 again. Thanks

[mgravell]

@AnthonyLloyd the problem there, though, is that I'm not very happy with running "deserialize" inside the sync-lock; I can fix that, but it involves moving a few more pieces; sigh

[amcasey]

CI failure is in Microsoft.Extensions.Caching.Hybrid.Tests.StampedeTests.MutableTypesNeverShareFinalTask(withCancelation: True), which looks related.

[amcasey]

The new comments look great - thanks! The new code looks non-trivial and I'll have to look at it more carefully. @BrennanConroy will probably be interested too (I think you directed his attention here in comments on the original PR).

[mgravell]

I'll look into the CI in the morning, ta