Skip to content

Commit 0e16dd1

Browse files
authored
chore: add singleflightcheck analyzer to enforce context-aware singleflight (#2954)
1 parent aa9e31d commit 0e16dd1

File tree

11 files changed

+200
-10
lines changed

11 files changed

+200
-10
lines changed

e2e/go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,5 +91,6 @@ require (
9191
google.golang.org/genproto/googleapis/rpc v0.0.0-20251124214823-79d6a2a48846 // indirect
9292
google.golang.org/protobuf v1.36.11 // indirect
9393
gopkg.in/yaml.v3 v3.0.1 // indirect
94+
resenje.org/singleflight v0.4.3 // indirect
9495
sigs.k8s.io/controller-runtime v0.22.4 // indirect
9596
)

internal/datastore/proxy/schemacaching/standardcache.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import (
66
"sync"
77
"unsafe"
88

9-
"golang.org/x/sync/singleflight"
9+
"resenje.org/singleflight"
1010

1111
"github.com/authzed/spicedb/pkg/cache"
1212
"github.com/authzed/spicedb/pkg/datastore"
@@ -20,7 +20,7 @@ import (
2020
type definitionCachingProxy struct {
2121
datastore.Datastore
2222
c cache.Cache[cache.StringKey, *cacheEntry]
23-
readGroup singleflight.Group
23+
readGroup singleflight.Group[string, *cacheEntry]
2424
}
2525

2626
func (p *definitionCachingProxy) Close() error {
@@ -176,8 +176,7 @@ func readAndCache[T schemaDefinition](
176176
loaded, found := r.p.c.Get(cache.StringKey(cacheRevisionKey))
177177
if !found {
178178
// We couldn't use the cached entry, load one
179-
var err error
180-
loadedRaw, err, _ := r.p.readGroup.Do(cacheRevisionKey, func() (any, error) {
179+
loadedEntry, _, err := r.p.readGroup.Do(ctx, cacheRevisionKey, func(ctx context.Context) (*cacheEntry, error) {
181180
// sever the context so that another branch doesn't cancel the
182181
// single-flighted read
183182
loaded, updatedRev, err := reader(context.WithoutCancel(ctx), name)
@@ -199,7 +198,7 @@ func readAndCache[T schemaDefinition](
199198
return *new(T), datastore.NoRevision, err
200199
}
201200

202-
loaded = loadedRaw.(*cacheEntry)
201+
loaded = loadedEntry
203202
}
204203

205204
return loaded.definition.(T), loaded.updated, loaded.notFound

internal/datastore/revisions/optimized.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import (
1010
"github.com/benbjohnson/clock"
1111
"go.opentelemetry.io/otel"
1212
"go.opentelemetry.io/otel/trace"
13-
"golang.org/x/sync/singleflight"
13+
"resenje.org/singleflight"
1414

1515
log "github.com/authzed/spicedb/internal/logging"
1616
"github.com/authzed/spicedb/internal/telemetry/otelconv"
@@ -63,12 +63,12 @@ func (cor *CachedOptimizedRevisions) OptimizedRevision(ctx context.Context) (dat
6363
}
6464
cor.RUnlock()
6565

66-
newQuantizedRevision, err, _ := cor.updateGroup.Do("", func() (any, error) {
66+
newQuantizedRevision, _, err := cor.updateGroup.Do(ctx, "", func(ctx context.Context) (datastore.Revision, error) {
6767
log.Ctx(ctx).Debug().Time("now", localNow).Msg("computing new revision")
6868

6969
optimized, validFor, err := cor.optimizedFunc(ctx)
7070
if err != nil {
71-
return nil, fmt.Errorf("unable to compute optimized revision: %w", err)
71+
return datastore.NoRevision, fmt.Errorf("unable to compute optimized revision: %w", err)
7272
}
7373

7474
rvt := localNow.Add(validFor)
@@ -95,7 +95,7 @@ func (cor *CachedOptimizedRevisions) OptimizedRevision(ctx context.Context) (dat
9595
if err != nil {
9696
return datastore.NoRevision, err
9797
}
98-
return newQuantizedRevision.(datastore.Revision), err
98+
return newQuantizedRevision, err
9999
}
100100

101101
// CachedOptimizedRevisions does caching and deduplication for requests for optimized revisions.
@@ -110,7 +110,7 @@ type CachedOptimizedRevisions struct {
110110
candidates []validRevision // GUARDED_BY(RWMutex)
111111

112112
// the updategroup consolidates concurrent requests to the database into 1
113-
updateGroup singleflight.Group
113+
updateGroup singleflight.Group[string, datastore.Revision]
114114
}
115115

116116
type validRevision struct {

magefiles/lint.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ func (Lint) Analyzers() error {
101101
"-protomarshalcheck",
102102
"-telemetryconvcheck",
103103
"-iferrafterrowclosecheck",
104+
"-singleflightcheck",
104105
// Skip generated protobuf files for this check
105106
// Also skip test where we're explicitly using proto.Marshal to assert
106107
// that the proto.Marshal behavior matches foo.MarshalVT()

tools/analyzers/cmd/analyzers/main.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"github.com/authzed/spicedb/tools/analyzers/nilvaluecheck"
1313
"github.com/authzed/spicedb/tools/analyzers/paniccheck"
1414
"github.com/authzed/spicedb/tools/analyzers/protomarshalcheck"
15+
"github.com/authzed/spicedb/tools/analyzers/singleflightcheck"
1516
"github.com/authzed/spicedb/tools/analyzers/telemetryconvcheck"
1617
"github.com/authzed/spicedb/tools/analyzers/zerologmarshalcheck"
1718
)
@@ -28,6 +29,7 @@ func main() {
2829
lendowncastcheck.Analyzer(),
2930
protomarshalcheck.Analyzer(),
3031
zerologmarshalcheck.Analyzer(),
32+
singleflightcheck.Analyzer(),
3133
telemetryconvcheck.Analyzer(),
3234
)
3335
}

tools/analyzers/go.work.sum

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.31.0-2023080216373
33
buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.36.4-20250130201111-63bb56e20495.1 h1:4erM3WLgEG/HIBrpBDmRbs1puhd7p0z7kNXDuhHthwM=
44
buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.36.4-20250130201111-63bb56e20495.1/go.mod h1:novQBstnxcGpfKf8qGRATqn1anQKwMJIbH5Q581jibU=
55
buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.36.6-20250425153114-8976f5be98c1.1/go.mod h1:avRlCjnFzl98VPaeCtJ24RrV/wwHFzB8sWXhj26+n/U=
6+
buf.build/go/hyperpb v0.1.3/go.mod h1:IHXAM5qnS0/Fsnd7/HGDghFNvUET646WoHmq1FDZXIE=
67
buf.build/go/protovalidate v0.12.0 h1:4GKJotbspQjRCcqZMGVSuC8SjwZ/FmgtSuKDpKUTZew=
78
buf.build/go/protovalidate v0.12.0/go.mod h1:q3PFfbzI05LeqxSwq+begW2syjy2Z6hLxZSkP1OH/D0=
89
cloud.google.com/go/accessapproval v1.7.1 h1:/5YjNhR6lzCvmJZAnByYkfEgWjfAKwYP6nkuTk6nKFE=
@@ -2545,6 +2546,7 @@ github.com/containerd/console v1.0.5 h1:R0ymNeydRqH2DmakFNdmjR2k0t7UPuiOV/N/27/q
25452546
github.com/containerd/console v1.0.5/go.mod h1:YynlIjWYF8myEu6sdkwKIvGQq+cOckRm6So2avqoYAk=
25462547
github.com/containerd/log v0.1.0 h1:TCJt7ioM2cr/tfR8GPbGf9/VRAX8D2B4PjzCpfX540I=
25472548
github.com/containerd/log v0.1.0/go.mod h1:VRRf09a7mHDIRezVKTRCrOq78v577GXq3bSa3EhrzVo=
2549+
github.com/containerd/typeurl/v2 v2.2.0/go.mod h1:8XOOxnyatxSWuG8OfsZXVnAF4iZfedjS/8UHSPJnX4g=
25482550
github.com/coreos/go-semver v0.3.0 h1:wkHLiw0WNATZnSG7epLsujiMCgPAc9xhjJ4tgnAxmfM=
25492551
github.com/coreos/go-semver v0.3.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk=
25502552
github.com/coreos/go-semver v0.3.1 h1:yi21YpKnrx1gt5R+la8n5WgS0kCrsPp33dmEyHReZr4=
@@ -2567,6 +2569,7 @@ github.com/cyphar/filepath-securejoin v0.3.5 h1:L81NHjquoQmcPgXcttUS9qTSR/+bXry6
25672569
github.com/cyphar/filepath-securejoin v0.3.5/go.mod h1:edhVd3c6OXKjUmSrVa/tGJRS9joFTxlslFCAyaxigkE=
25682570
github.com/cyphar/filepath-securejoin v0.5.1 h1:eYgfMq5yryL4fbWfkLpFFy2ukSELzaJOTaUTuh+oF48=
25692571
github.com/cyphar/filepath-securejoin v0.5.1/go.mod h1:Sdj7gXlvMcPZsbhwhQ33GguGLDGQL7h7bg04C/+u9jI=
2572+
github.com/docker/docker v28.0.0+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
25702573
github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815 h1:bWDMxwH3px2JBh6AyO7hdCn/PkvCZXii8TGj7sbtEbQ=
25712574
github.com/ebitengine/purego v0.8.1 h1:sdRKd6plj7KYW33EH5As6YKfe8m9zbN9JMrOjNVF/BE=
25722575
github.com/ebitengine/purego v0.8.1/go.mod h1:iIjxzd6CiRiOG0UyXP+V1+jWqUXVjPKLAI0mRfJZTmQ=
@@ -2850,6 +2853,7 @@ github.com/sagikazarmark/crypt v0.15.0 h1:TQJg76CemcIdJyC9/dmNjU9OUyIFHyvE50Tpq1
28502853
github.com/sagikazarmark/crypt v0.15.0/go.mod h1:5rwNNax6Mlk9sZ40AcyVtiEw24Z4J04cfSioF2COKmc=
28512854
github.com/sagikazarmark/crypt v0.17.0 h1:ZA/7pXyjkHoK4bW4mIdnCLvL8hd+Nrbiw7Dqk7D4qUk=
28522855
github.com/sagikazarmark/crypt v0.17.0/go.mod h1:SMtHTvdmsZMuY/bpZoqokSoChIrcJ/epOxZN58PbZDg=
2856+
github.com/santhosh-tekuri/jsonschema/v5 v5.3.1/go.mod h1:uToXkOrWAZ6/Oc07xWQrPOhJotwFIyu2bBVN41fcDUY=
28532857
github.com/seccomp/libseccomp-golang v0.9.2-0.20220502022130-f33da4d89646 h1:RpforrEYXWkmGwJHIGnLZ3tTWStkjVVstwzNGqxX2Ds=
28542858
github.com/seccomp/libseccomp-golang v0.9.2-0.20220502022130-f33da4d89646/go.mod h1:JA8cRccbGaA1s33RQf7Y1+q9gHmZX1yB/z9WDN1C6fg=
28552859
github.com/seccomp/libseccomp-golang v0.10.0 h1:aA4bp+/Zzi0BnWZ2F1wgNBs5gTpm+na2rWM6M9YjLpY=
@@ -2908,6 +2912,7 @@ github.com/tidwall/pretty v1.2.1 h1:qjsOFOWWQl+N3RsoF5/ssm1pHmJJwhjlSbZ51I6wMl4=
29082912
github.com/tidwall/pretty v1.2.1/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU=
29092913
github.com/tidwall/sjson v1.2.5 h1:kLy8mja+1c9jlljvWTlSazM7cKDRfJuR/bOJhcY5NcY=
29102914
github.com/tidwall/sjson v1.2.5/go.mod h1:Fvgq9kS/6ociJEDnK0Fk1cpYF4FIW6ZF7LAe+6jwd28=
2915+
github.com/timandy/routine v1.1.6/go.mod h1:kXslgIosdY8LW0byTyPnenDgn4/azt2euufAq9rK51w=
29112916
github.com/tklauser/go-sysconf v0.3.11 h1:89WgdJhk5SNwJfu+GKyYveZ4IaJ7xAkecBo+KdJV0CM=
29122917
github.com/tklauser/go-sysconf v0.3.11/go.mod h1:GqXfhXY3kiPa0nAXPDIQIWzJbMCB7AmcWpGR8lSZfqI=
29132918
github.com/tklauser/go-sysconf v0.3.12 h1:0QaGUFOdQaIVdPgfITYzaTegZvdCjmYO52cSFAEVmqU=
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
package singleflightcheck
2+
3+
import (
4+
"flag"
5+
"fmt"
6+
"go/ast"
7+
"regexp"
8+
"strings"
9+
10+
"github.com/samber/lo"
11+
"golang.org/x/tools/go/analysis"
12+
"golang.org/x/tools/go/analysis/passes/inspect"
13+
"golang.org/x/tools/go/ast/inspector"
14+
)
15+
16+
func Analyzer() *analysis.Analyzer {
17+
flagSet := flag.NewFlagSet("singleflightcheck", flag.ExitOnError)
18+
skipPkg := flagSet.String("skip-pkg", "", "package(s) to skip for linting")
19+
skipFiles := flagSet.String("skip-files", "", "patterns of files to skip for linting")
20+
21+
return &analysis.Analyzer{
22+
Name: "singleflightcheck",
23+
Doc: "reports uses of golang.org/x/sync/singleflight.Group.Do in functions that have a context.Context parameter; use resenje.org/singleflight instead",
24+
Run: func(pass *analysis.Pass) (any, error) {
25+
// Check for a skipped package.
26+
if len(*skipPkg) > 0 {
27+
skipped := lo.Map(strings.Split(*skipPkg, ","), func(skipped string, _ int) string { return strings.TrimSpace(skipped) })
28+
for _, s := range skipped {
29+
if strings.Contains(pass.Pkg.Path(), s) {
30+
return nil, nil
31+
}
32+
}
33+
}
34+
35+
// Check for a skipped file.
36+
skipFilePatterns := make([]string, 0)
37+
if len(*skipFiles) > 0 {
38+
skipFilePatterns = lo.Map(strings.Split(*skipFiles, ","), func(skipped string, _ int) string { return strings.TrimSpace(skipped) })
39+
}
40+
for _, pattern := range skipFilePatterns {
41+
_, err := regexp.Compile(pattern)
42+
if err != nil {
43+
return nil, fmt.Errorf("invalid skip-files pattern `%s`: %w", pattern, err)
44+
}
45+
}
46+
47+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
48+
49+
nodeFilter := []ast.Node{
50+
(*ast.File)(nil),
51+
(*ast.CallExpr)(nil),
52+
}
53+
54+
inspect.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool {
55+
switch s := n.(type) {
56+
case *ast.File:
57+
for _, pattern := range skipFilePatterns {
58+
isMatch, _ := regexp.MatchString(pattern, pass.Fset.Position(s.Package).Filename)
59+
if isMatch {
60+
return false
61+
}
62+
}
63+
return true
64+
65+
case *ast.CallExpr:
66+
// Check if this is a call to .Do on a selector expression.
67+
selector, ok := s.Fun.(*ast.SelectorExpr)
68+
if !ok || selector.Sel.Name != "Do" {
69+
return true
70+
}
71+
72+
// Check that the receiver type is golang.org/x/sync/singleflight.Group.
73+
receiverType := pass.TypesInfo.TypeOf(selector.X)
74+
if receiverType == nil {
75+
return true
76+
}
77+
78+
typeStr := receiverType.String()
79+
if !strings.Contains(typeStr, "golang.org/x/sync/singleflight") {
80+
return true
81+
}
82+
83+
// Check if the enclosing function has a context.Context parameter.
84+
if !enclosingFuncHasContext(stack) {
85+
return true
86+
}
87+
88+
pass.Reportf(n.Pos(), "In package %s: use resenje.org/singleflight instead of golang.org/x/sync/singleflight in functions with context.Context", pass.Pkg.Path())
89+
return false
90+
91+
default:
92+
return true
93+
}
94+
})
95+
96+
return nil, nil
97+
},
98+
Requires: []*analysis.Analyzer{inspect.Analyzer},
99+
Flags: *flagSet,
100+
}
101+
}
102+
103+
func enclosingFuncHasContext(stack []ast.Node) bool {
104+
for i := len(stack) - 1; i >= 0; i-- {
105+
var params *ast.FieldList
106+
switch f := stack[i].(type) {
107+
case *ast.FuncDecl:
108+
params = f.Type.Params
109+
case *ast.FuncLit:
110+
params = f.Type.Params
111+
default:
112+
continue
113+
}
114+
115+
if params == nil {
116+
return false
117+
}
118+
119+
for _, param := range params.List {
120+
if sel, ok := param.Type.(*ast.SelectorExpr); ok {
121+
if ident, ok := sel.X.(*ast.Ident); ok {
122+
if ident.Name == "context" && sel.Sel.Name == "Context" {
123+
return true
124+
}
125+
}
126+
}
127+
}
128+
return false
129+
}
130+
return false
131+
}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
package singleflightcheck
2+
3+
import (
4+
"testing"
5+
6+
"golang.org/x/tools/go/analysis/analysistest"
7+
)
8+
9+
func TestAnalyzer(t *testing.T) {
10+
analyzer := Analyzer()
11+
12+
testdata := analysistest.TestData()
13+
analysistest.Run(t, testdata, analyzer, "badsingleflight")
14+
analysistest.Run(t, testdata, analyzer, "goodsingleflight")
15+
}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
package badsingleflight
2+
3+
import (
4+
"context"
5+
6+
"golang.org/x/sync/singleflight"
7+
)
8+
9+
var group singleflight.Group
10+
11+
func HasContext(ctx context.Context) {
12+
group.Do("key", func() (any, error) { // want "use resenje.org/singleflight instead of golang.org/x/sync/singleflight in functions with context.Context"
13+
return nil, nil
14+
})
15+
}
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
package singleflight
2+
3+
type Group struct{}
4+
5+
func (g *Group) Do(key string, fn func() (any, error)) (any, error, bool) {
6+
v, err := fn()
7+
return v, err, false
8+
}

0 commit comments

Comments
 (0)