pkg/locks.NamedLocker: review usage and implementation

[crandles]

Review usage of the pkg/locks.NamedLocker construct for improvements in addition to the NamedLocker implementation for optimization / improvements.

[crandles]

#819 simplified named locker usage by:

• moving usage from each individual cache client to a shared layer.

#834 also optimizes named locker usage by avoiding it for duplicate lookup calls

[jranson]

it seems like locks.NamedLocker might be a good candidate for complete removal/replacement by the new golang.org/x/sync/singleflight package, since all NamedLocker does is queue up concurrent requests for lookups to the same cache key so only one lookup happens - exactly what singleflight does.

[crandles]

I think in many cases that is true we might be able to drop the named locker -- although I don't think it was queueing up requests so much as synchronizing reads and writes (multiple reads could happen at the same time).

E.g. for our cache clients, we can generally trust that the backend cache will return the correct data, even if a write is ongoing.

Though, this may not be the case for filesystem cache. Perhaps that's the only exception, as far as the cache clients. It could probably use a simpler lock implementation.

I do see three other usages of the named locker, across:

• https://github.com/trickstercache/trickster/blob/main/pkg%2Fproxy%2Fengines%2Fobjectproxycache.go
• https://github.com/trickstercache/trickster/blob/main/pkg%2Fproxy%2Fengines%2Fdeltaproxycache.go

[jranson]

Ah, NamedLocker does provide a read lock. However, in addition to Filesystem, that also might not be the best thing for remote caches like Redis. Our NamedLockers currently allow a thundering herd into Redis for the same cache key/data, while a singleflight Group would not. That could impact not only performance, but also cloud spend $$ with solutions like AWS Elasticache that charge per # of lookups.

So we probably just want to think about how to re-approach (A) cache key lookup concurrency synchronization (single lookup for all concurrent requests to the same cache key, that simultaneously receive the same result of hit+payload or miss ; remove concurrent reads) and separately (B) subsequent cache miss handling (single outbound request or sharded requests group that - after reconstituting any shards - can share the response io.Reader stream with the RequestWriters of all the concurrents + the cache) and finally (C) storing the cache entry -- all as separate steps that may be better synchronized independently of each other.

Right now to get from A to B, in the event of a cache miss, we "upgrade" NamedLocker's RLock to an RWLock and then proceed with handling the cache miss guarded by the upgraded RWLock. One problematic area there is that any outbound requests required to fulfill a cache miss are not initiated until locker.Upgrade() unblocks. If I were re-writing this, I would avoid gating the ability to fetch on a cache miss by locker.Upgrade(), and instead initiate a singleflight-like outbound request as soon as the cache miss is returned. Then, acquire a cache write lock before writing back to cache. End goal being - the only time the cache key is locked is when reading/writing to it and not during the time in between when the cache miss is being fulfilled, which uses its own syncs. Requests coming in during fulfillment could get a cache miss from A, and just join the singleflight-like group in B. We'll need to consider how/when to close the B singleflight-like group (e.g., wait until after the cache write is completed + key unlocked) to avoid a race condition where a request gets a cache miss in A, but by the time it gets to B the singleflight-like group for the cache miss has finished and closed.