HybridCache - minor post-PR fixes by mgravell · Pull Request #55251 · dotnet/aspnetcore (original) (raw)

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.