Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loopvarcapture linter should check interface methods #102678

Closed
srosenberg opened this issue May 1, 2023 · 2 comments · Fixed by #102679
Closed

loopvarcapture linter should check interface methods #102678

srosenberg opened this issue May 1, 2023 · 2 comments · Fixed by #102679
Labels
A-testing Testing tools and infrastructure C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-testeng TestEng Team

Comments

@srosenberg
Copy link
Member

srosenberg commented May 1, 2023

The existing implementation of the loopvarcapture linter [1] incorrectly omits all checks inside Monitor.Go [2]. This was a fairly random discovery. I was performing a manual inspection of the output of the go compiler, using -gcflags="-m=2". Surprisingly, I noticed the following diagnostic message. Upon closer inspection, it's indeed a bug which was supposed to be flagged by the loopvarcapture linter.

pkg/cmd/roachtest/tests/kv.go:759:7: registerKVScalability.func1 capturing by ref: i (addr=false assign=true width=8)

The root cause is restricting callees (of Go variants) to be non-interface methods. The callee in [2] happens to be an interface method.

[1] #80803
[2]

{Pkg: "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster", Type: "Monitor", Name: "Go"},

Jira issue: CRDB-27587

@srosenberg srosenberg added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-testing Testing tools and infrastructure labels May 1, 2023
@blathers-crl blathers-crl bot added the T-testeng TestEng Team label May 1, 2023
@blathers-crl
Copy link

blathers-crl bot commented May 1, 2023

cc @cockroachdb/test-eng

@srosenberg
Copy link
Member Author

srosenberg commented May 1, 2023

After instrumenting the linter to print matched Go variants (inside isGoRoutineFunction), we can confirm the missing checks as follows.

Build roachvet

./dev build //pkg/cmd/roachvet

roachvet analysis results (before fix)

go vet -vettool _bazel/bin/pkg/cmd/roachvet/roachvet_/roachvet -tags stdmalloc ./... >/tmp/loopvarcapture.out 2>&1

grep isGoRoutineFunction /tmp/loopvarcapture.out |sed -E "s|^.+call=([^,]+).+$|\1|g"|sort |uniq -c |sort -rn
  78 golang.org/x/sync/errgroup.Group.Go
  57 github.com/cockroachdb/cockroach/pkg/util/ctxgroup.Group.GoCtx
   5 github.com/cockroachdb/cockroach/pkg/util/ctxgroup.Group.Go

roachvet analysis results (after fix)

go vet -vettool _bazel/bin/pkg/cmd/roachvet/roachvet_/roachvet -tags stdmalloc ./... >/tmp/loopvarcapture.out 2>&1

grep isGoRoutineFunction /tmp/loopvarcapture.out |sed -E "s|^.+call=([^,]+).+$|\1|g"|sort |uniq -c |sort -rn
  78 golang.org/x/sync/errgroup.Group.Go
  57 github.com/cockroachdb/cockroach/pkg/util/ctxgroup.Group.GoCtx
  28 github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster.Monitor.Go
   5 github.com/cockroachdb/cockroach/pkg/util/ctxgroup.Group.Go

Thus, we were missing 28 occurrences inside Monitor.Go, one of which contains a bug, introduced ~5 years ago:

for i := nodes; i <= nodes*maxPerNodeConcurrency; i += nodes {
c.Wipe(ctx, c.Range(1, nodes))
c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), c.Range(1, nodes))
t.Status("running workload")
m := c.NewMonitor(ctx, c.Range(1, nodes))
m.Go(func(ctx context.Context) error {
cmd := fmt.Sprintf("./workload run kv --init --read-percent=%d "+
"--splits=1000 --duration=1m "+fmt.Sprintf("--concurrency=%d", i)+

craig bot pushed a commit that referenced this issue May 11, 2023
102679: linter: loopvarcapture should check interface methods r=renatolabs,herkolategan a=srosenberg

The `loopvarcapture` linter is part of `roachvet`. It flags incorrect uses of loop variables captured by reference in Go routines or defer statements--a common source of data races.

In addition to checking Go routines created via the `go` keyword, the linter also checks against other (internal) APIs, which are known to create Go routines; this is specified via `GoRoutineFunctions`.

An existing bug prevented the linter from checking inside `Monitor.Go`, which are commonly used inside roachtests.
This PR fixes the linter bug as well as the pre-existing bug inside a roachtest, which this linter now detects.

This PR also removes the uncessary hack, i.e., `RunDespiteErrors`, introduced in [1]. By allowing the type checker to swallow errors, the test was susceptible to silent errors, e.g., buggy testdata. Instead, we work around by disabling CGO.

Epic: none
Fixes: #102678
Release note: None

[1] #84867 (comment)

Co-authored-by: Stan Rosenberg <[email protected]>
@craig craig bot closed this as completed in 187ae85 May 11, 2023
blathers-crl bot pushed a commit that referenced this issue May 11, 2023
The `loopvarcapture` linter is part of `roachvet`.
It flags incorrect uses of loop variables captured by reference
in Go routines or defer statements--a common source of data races.

In addition to checking Go routines created via the `go` keyword,
the linter also checks against other (internal) APIs, which are known
to create Go routines; this is specified via `GoRoutineFunctions`.

An existing bug prevented the linter from checking inside `Monitor.Go`,
which are commonly used inside roachtests.
This PR fixes the linter bug as well as the pre-existing bug inside
a roachtest, which this linter now detects.

This PR also removes the uncessary hack, i.e., `RunDespiteErrors`, introduced
in [1]. By allowing the type checker to swallow errors, the test was susceptible
to silent errors, e.g., buggy testdata. Instead, we work around by disabling CGO.

Epic: none
Fixes: #102678
Release note: None

[1] #84867 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Testing tools and infrastructure C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-testeng TestEng Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant