chore: add singleflightcheck analyzer to enforce context-aware singleflight#2954
Conversation
github-actions
bot
added
area/datastore
josephschorr
force-pushed
the
singleflight-ctx
branch
2 times, most recently
from
2d46179 to
44b9aa8
Compare
| // We couldn't use the cached entry, load one | ||
| var err error | ||
| loadedRaw, err, _ := r.p.readGroup.Do(cacheRevisionKey, func() (any, error) { | ||
| loadedEntry, _, err := r.p.readGroup.Do(ctx, cacheRevisionKey, func(ctx context.Context) (*cacheEntry, error) { |
There was a problem hiding this comment.
For this and the other callsite: we're now passing the context which means it can be cancelled, but do we know that both of these codepaths have timeouts on them somewhere further up the chain?
There was a problem hiding this comment.
i'm wondering why we didn't act defensively and added a context timeout anyway..
There was a problem hiding this comment.
i'm wondering why we didn't act defensively and added a context timeout anyway..
That was my proposal: we wrap this (and any other singleflight) calls in a timeout anyway, but since we didn't get consensus, I did this portion first
There was a problem hiding this comment.
but do we know that both of these codepaths have timeouts on them somewhere further up the chain?
Yes, all requests have timeouts ultimately from the API layer that moves downward into here (except for writes, which break the context, but have their own timeouts)
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (74.83%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2954 +/- ##
==========================================
+ Coverage 74.77% 74.83% +0.06%
==========================================
Files 494 494
Lines 60623 60622 -1
==========================================
+ Hits 45323 45358 +35
+ Misses 12136 12115 -21
+ Partials 3164 3149 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "-protomarshalcheck", | ||
| "-telemetryconvcheck", | ||
| "-iferrafterrowclosecheck", | ||
| "-singleflightcheck", |
There was a problem hiding this comment.
use depguard please
settings:
depguard:
rules:
main:
deny:
- pkg: "golang.org/x/sync/singleflight"
desc: "use resenje.org/singleflight instead so that we can break out of deadlocks"
There was a problem hiding this comment.
I think the idea was that there may be places where you want to singleflight and don't have access to a context object. Unless we're saying that we'd want to create a context with timeout at that callsite?
There was a problem hiding this comment.
We can't use depguard: there are times when we need to use singleflight without context being available; the linter only checks the cases where context is available
tstirrat15
force-pushed
the
singleflight-ctx
branch
from
44b9aa8 to
d27c4fe
Compare
…flight Add a custom analyzer that detects uses of golang.org/x/sync/singleflight in functions with a context.Context parameter, requiring resenje.org/singleflight instead. The resenje variant is context-aware and supports generics, preventing context cancellation from being silently ignored in singleflighted calls. Migrate the two remaining callsites (optimized revisions and schema caching) to resenje.org/singleflight. Fixes authzed#2953
tstirrat15
force-pushed
the
singleflight-ctx
branch
from
d27c4fe to
0cb63dc
Compare
3 participants
Adds a custom analyzer that detects uses of golang.org/x/sync/singleflight in functions with a context.Context parameter, requiring resenje.org/singleflight instead. The resenje variant is context-aware and supports generics, preventing context cancellation from being silently ignored in singleflighted calls.
Migrate the two remaining callsites (optimized revisions and schema caching) to resenje.org/singleflight.
Fixes #2953