chore: add singleflightcheck analyzer to enforce context-aware singleflight #2954
+200
−10
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| package singleflightcheck | ||
|
|
||
| import ( | ||
| "flag" | ||
| "fmt" | ||
| "go/ast" | ||
| "regexp" | ||
| "strings" | ||
|
|
||
| "github.com/samber/lo" | ||
| "golang.org/x/tools/go/analysis" | ||
| "golang.org/x/tools/go/analysis/passes/inspect" | ||
| "golang.org/x/tools/go/ast/inspector" | ||
| ) | ||
|
|
||
| func Analyzer() *analysis.Analyzer { | ||
| flagSet := flag.NewFlagSet("singleflightcheck", flag.ExitOnError) | ||
| skipPkg := flagSet.String("skip-pkg", "", "package(s) to skip for linting") | ||
| skipFiles := flagSet.String("skip-files", "", "patterns of files to skip for linting") | ||
|
|
||
| return &analysis.Analyzer{ | ||
| Name: "singleflightcheck", | ||
| Doc: "reports uses of golang.org/x/sync/singleflight.Group.Do in functions that have a context.Context parameter; use resenje.org/singleflight instead", | ||
| Run: func(pass *analysis.Pass) (any, error) { | ||
| // Check for a skipped package. | ||
| if len(*skipPkg) > 0 { | ||
| skipped := lo.Map(strings.Split(*skipPkg, ","), func(skipped string, _ int) string { return strings.TrimSpace(skipped) }) | ||
| for _, s := range skipped { | ||
| if strings.Contains(pass.Pkg.Path(), s) { | ||
| return nil, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check for a skipped file. | ||
| skipFilePatterns := make([]string, 0) | ||
| if len(*skipFiles) > 0 { | ||
| skipFilePatterns = lo.Map(strings.Split(*skipFiles, ","), func(skipped string, _ int) string { return strings.TrimSpace(skipped) }) | ||
| } | ||
| for _, pattern := range skipFilePatterns { | ||
| _, err := regexp.Compile(pattern) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid skip-files pattern `%s`: %w", pattern, err) | ||
| } | ||
| } | ||
|
|
||
| inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) | ||
|
|
||
| nodeFilter := []ast.Node{ | ||
| (*ast.File)(nil), | ||
| (*ast.CallExpr)(nil), | ||
| } | ||
|
|
||
| inspect.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool { | ||
| switch s := n.(type) { | ||
| case *ast.File: | ||
| for _, pattern := range skipFilePatterns { | ||
| isMatch, _ := regexp.MatchString(pattern, pass.Fset.Position(s.Package).Filename) | ||
| if isMatch { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
|
|
||
| case *ast.CallExpr: | ||
| // Check if this is a call to .Do on a selector expression. | ||
| selector, ok := s.Fun.(*ast.SelectorExpr) | ||
| if !ok || selector.Sel.Name != "Do" { | ||
| return true | ||
| } | ||
|
|
||
| // Check that the receiver type is golang.org/x/sync/singleflight.Group. | ||
| receiverType := pass.TypesInfo.TypeOf(selector.X) | ||
| if receiverType == nil { | ||
| return true | ||
| } | ||
|
|
||
| typeStr := receiverType.String() | ||
| if !strings.Contains(typeStr, "golang.org/x/sync/singleflight") { | ||
| return true | ||
| } | ||
|
|
||
| // Check if the enclosing function has a context.Context parameter. | ||
| if !enclosingFuncHasContext(stack) { | ||
| return true | ||
| } | ||
|
|
||
| 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()) | ||
| return false | ||
|
|
||
| default: | ||
| return true | ||
| } | ||
| }) | ||
|
|
||
| return nil, nil | ||
| }, | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Flags: *flagSet, | ||
| } | ||
| } | ||
|
|
||
| func enclosingFuncHasContext(stack []ast.Node) bool { | ||
| for i := len(stack) - 1; i >= 0; i-- { | ||
| var params *ast.FieldList | ||
| switch f := stack[i].(type) { | ||
| case *ast.FuncDecl: | ||
| params = f.Type.Params | ||
| case *ast.FuncLit: | ||
| params = f.Type.Params | ||
| default: | ||
| continue | ||
| } | ||
|
|
||
| if params == nil { | ||
| return false | ||
| } | ||
|
|
||
| for _, param := range params.List { | ||
| if sel, ok := param.Type.(*ast.SelectorExpr); ok { | ||
| if ident, ok := sel.X.(*ast.Ident); ok { | ||
| if ident.Name == "context" && sel.Sel.Name == "Context" { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } | ||
| return false | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package singleflightcheck | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| analyzer := Analyzer() | ||
|
|
||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, analyzer, "badsingleflight") | ||
| analysistest.Run(t, testdata, analyzer, "goodsingleflight") | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package badsingleflight | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "golang.org/x/sync/singleflight" | ||
| ) | ||
|
|
||
| var group singleflight.Group | ||
|
|
||
| func HasContext(ctx context.Context) { | ||
| group.Do("key", func() (any, error) { // want "use resenje.org/singleflight instead of golang.org/x/sync/singleflight in functions with context.Context" | ||
| return nil, nil | ||
| }) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package singleflight | ||
|
|
||
| type Group struct{} | ||
|
|
||
| func (g *Group) Do(key string, fn func() (any, error)) (any, error, bool) { | ||
| v, err := fn() | ||
| return v, err, false | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package goodsingleflight | ||
|
|
||
| import ( | ||
| "golang.org/x/sync/singleflight" | ||
| ) | ||
|
|
||
| var group singleflight.Group | ||
|
|
||
| func NoContext() { | ||
| group.Do("key", func() (any, error) { | ||
| return nil, nil | ||
| }) | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm wondering why we didn't act defensively and added a context timeout anyway..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)