Proposal - Pluggable cache abstraction#142
Conversation
f2fed3c to
c221cd0
Compare
| } | ||
|
|
||
| //TODO update this with stampede protection at some point. | ||
| public async ValueTask<T> GetOrSet<T>(string key, Func<CancellationToken, Task<T>> factory, TimeSpan? ttlOverride = null, CancellationToken token = default) |
There was a problem hiding this comment.
do we need this, i don't think it's called anywhere. if so i believe there's an issue with the implementation.
GetOrSet has a latent bug for value types. The miss check relies on Get returning null on a miss, but T? on an unconstrained generic isn't Nullable — it's just T with a nullability annotation. On a miss Get returns default(T), so GetOrSet("k", factory) returns 0 and never invokes the factory. Reference types are fine (which is everything we cache today), so it's latent, not actively broken. Simplest fix is where T : class on GetOrSet — still permits all current usage. Or switch to TryGet-style presence detection if you want to keep value types in scope.
There was a problem hiding this comment.
This was added as most .NET cache abstractions include a factory GetOrSet out of the box, and it might get use down at the road for stampede protection. A good example is the CheckFlagWithEntitlementApi implementation, that currently does the cache check and API request separately. If you had concurrent calls for the same flag key and it was a cache miss, they would all attempt an API request instead of waiting for the first request to complete.
The value type issue is correct, and adding a class constraint didn't make a lot of sense as a chunk of the existing tests use value types. I've updated both implementations to better support value types.
Hiya,
I'm looking at building some more advanced abstractions around this package in order to automate feature tracking & checking in ASP.NET Core, but at the moment this package doesn't integrate well with the wider ecosystem.
Like many modern .NET projects, we already have a L1 + L2 + Backplane caching solution via FusionCache, and I'd prefer to reuse all of that infrastructure to keep our nodes in sync as it's far less chatty than a pure redis cache. At the moment there isn't an easy to way to swap out the caching implementation globally, and so that's what I've proposed here.
The main change is the cache abstraction is provided as a non-generic interface and reused where required, rather than having a cache per-type which doesn't scale well.
The existing
RedisCachealso had a few issues that've been fixed as part of this:ConnectionMultiplexeris designed to be created once for the life of an application and reused where needed, not created per-cache. Projects that are already using Redis will have a pre-configured Multiplexer and libraries typically let users provide one rather than dealing with the Redis connection themselves, so most of the connection options have been made obsolete.Edit: I did another pass at the configuration options and aligned them with Microsoft's own Redis options for flexibility & familiarity.