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

Jump to bottom

cmd/vet: extend the loopclosure analysis to parallel subtests #55972

Closed
findleyr opened this issue Sep 30, 2022 · 9 comments
Closed

cmd/vet: extend the loopclosure analysis to parallel subtests #55972

findleyr opened this issue Sep 30, 2022 · 9 comments
Assignees
@findleyr
Labels
Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Milestone
Go1.20

Comments

@findleyr
Copy link
Contributor

findleyr commented Sep 30, 2022 •
edited

I propose to update the loopclosure vet analyzer to check for references to loop variables from within parallel subtests. For example, with this change the analyzer would report a diagnostic for the use of tc after the call to t.Parallel() in the subtest body below:

https://go.dev/play/p/vekL_ZuhIpn

func TestLoopclosure(t *testing.T) {
	tests := []string{"a", "b", "c"}
	for _, tc := range tests {
		// Without this declaration, the test passes.
		// tc := tc
		t.Run(tc, func(t *testing.T) {
			t.Parallel()
			if tc != "c" {
				t.Errorf(`got %s, want "c"`, tc)
			}
		})
	}
}

Recently, @drchase has been experimenting with loop iterator capture semantics, and one of the outcomes of this experiment on large codebases is that parallel subtests frequently start failing, because they were not actually running each subtest due to the bug demonstrated above.

There is precedent for extending the loopclosure analyzer to recognize package APIs that are known to be concurrent: it already recognizes calls to x/sync/errgroup.Group.Go as invoking their argument concurrently. The case of parallel subtests is only slightly more complicated: look for calls to testing.T.Run that invoke T.Parallel. We added this in x/tools, guarded behind an internal variable, and the new check can be experimented with by building the command x/tools/internal/loopclosure, which enables the new check (caveat: the current logic could still be refined, for example by verifying that the receiver of t.Parallel matches the t passed in by t.Run).

A couple details about how this would be implemented:

  • The check would only consider statements that occur after the first call to t.Parallel within a function literal argument to t.Run. Users can and do rely on the fact that statements before the call to t.Parallel are executed synchronously.
  • optional: Whereas the loopclosure check for go, defer, and errgroup.Group.Go only consider invocations in the last statement of the loop, this check could consider all invocations of t.Run within the loop body. Unlike with go, defer, and errgroup.Group.Go, there is no common pattern for synchronizing parallel subtests. Attempting to do so would be problematic, as one would be fighting with the testing framework. (Note: it's also fine to not expand the check in this way, as I expect the overwhelming majority of parallel subtests are executed as the last statement in the loop body).

We found ~hundreds of bugs inside Google with this new check, and at least one user on Slack who noticed the in-progress change reported it as having found bugs in their codebase. We can do more research, but I suspect this new analysis would be accurate, actionable, and frequently prevent bugs.

CC @adonovan @lfolger @timothy-king
@gopherbot gopherbot added this to the Proposal milestone Sep 30, 2022
@timothy-king
Copy link
Contributor

Seems like a great idea to me overall. Minor comments about implementation details.

Whereas the loopclosure check for go, defer, and errgroup.Group.Go only consider invocations in the last statement of the loop, this check could consider all invocations of t.Run within the loop body. Unlike with go, defer, and errgroup.Group.Go, there is no common pattern for synchronizing parallel subtests.

There is one where an additional t.Run is used as parentheses to say when a group of parallel tests finishes: example https://go.dev/play/p/SZmQsSkFo8L and docs https://cs.opensource.google/go/go/+/refs/tags/go1.19.1:src/testing/testing.go;l=326-337;drc=690851ee3e48b85088698a9dbcc7dd112cef9eb5 . We probably should handle these ()'s. I don't think we need to worry about matching the pointer values of the *T.Testing objects, i.e. the parent of the receiver in T.Parallel matches the receiver of the immediately containing T.Run. This does not seem like an interesting detail to need to match.

The check would only consider statements that occur after the first call to t.Parallel within a function literal argument to t.Run.

To know if it is a race this should probably only be statements/expressions dominated by a t.Parallel() call. A very fast check with high precision is to just look at the statements in the range block (current implementation). I expect this to be somewhat acceptable as most reasonable usage is for it to be the first 1-2 statements. I think it is acceptable to not worry too hard about goto statements.

An alternative check is any usage reachable from a t.Parallel(). However this seems like a not helpful frequency improvement for a noticeable precision loss. (Imagine having conditional t.Parallel statements with conditional reads of variables.)

@findleyr
Copy link
Contributor Author

There is one where an additional t.Run is used as parentheses to say when a group of parallel tests finishes:

Thanks for pointing that out. In the current implementation, we only consider t.Run statements that are at the at the top level of the loop body, and therefore wouldn't check the bodies of those sub-sub-tests. In other words, we won't produce false positives in that case (currently), but also wouldn't produce true positives. That seems OK to me, but we could certainly do more.

To know if it is a race this should probably only be statements/expressions dominated by a t.Parallel() call. A very fast check with high precision is to just look at the statements in the range block (current implementation).

Another great point. I suspect that just looking at statements in the loop body is probably good enough, and additional precision is not worth the complexity it would entail. I'll admit I hadn't sufficiently considered the case of goto (e.g. https://go.dev/play/p/8u87MIesRHC). It may be worthwhile to ignore any subtests that have labels.

@timothy-king
Copy link
Contributor

In the current implementation, we only consider t.Run statements that are at the at the top level of the loop body, and therefore wouldn't check the bodies of those sub-sub-tests.

This seems reasonable to me. It may be good to document and/or add tests to ensure the very tempting switch from ranging over the body's statements to ast.Inspect to find T.Run/T.Parallel does not start introducing false positives.

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 20, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: cmd/vet: extend the loopclosure analysis to parallel subtests cmd/vet: extend the loopclosure analysis to parallel subtests Oct 26, 2022
@rsc rsc modified the milestones: Proposal, Backlog Oct 26, 2022
@findleyr findleyr self-assigned this Oct 27, 2022
@gopherbot
Copy link

Change https://go.dev/cl/446874 mentions this issue: go/analysis/passes/loopclosure: enable analysis of parallel subtests

@gopherbot
Copy link

Change https://go.dev/cl/447256 mentions this issue: go/analysis/passes/loopclosure: enable analysis of parallel subtests

@thepudds thepudds mentioned this issue Nov 4, 2022
Open
This was referenced Nov 16, 2022
Closed
Closed
thepudds added a commit to thepudds/xtools that referenced this issue Nov 18, 2022
@thepudds
go/analysis/passes/loopclosure: recursively check last statements in …
259b411 
…statements like if, switch, and for

In golang/go#16520, Allan Donovan suggested the current loopclosure
check could be extended to check more statements.

The current loopclosure flags patterns like:

	for k, v := range seq {
		go/defer func() {
			... k, v ...
		}()
	}

The motivating example for this CL from golang/go#16520 was:

	var wg sync.WaitGroup
	for i := 0; i < 10; i++ {
		for j := 0; j < 1; j++ {
			wg.Add(1)
			go func() {
				fmt.Printf("%d ", i)
				wg.Done()
			}()
		}
	}
	wg.Wait()

The current loopclosure check does not flag this because of the
inner for loop, and the checker looks only at the last statement
in the outer loop body.

Allan suggested we redefine "last" recursively. For example,
if the last statement is an if, then we examine the last statements
in both of its branches. Or if the last statement is a nested loop,
then we examine the last statement of that loop's body, and so on.

A few years ago, Allan sent a sketch in CL 184537.

This CL attempts to complete Allan's sketch, as well as integrates
with the ensuing changes from golang/go#55972 to check errgroup.Group.Go,
which with this CL can now be recursively "last".

Updates golang/go#16520
Updates golang/go#55972
@thepudds thepudds mentioned this issue Nov 18, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/452155 mentions this issue: go/analysis/passes/loopclosure: recursively check last statements in statements like if, switch, and for

gopherbot pushed a commit to golang/tools that referenced this issue Nov 21, 2022
@thepudds @timothy-king
go/analysis/passes/loopclosure: recursively check last statements in …
b797704 
…statements like if, switch, and for

In golang/go#16520, there was a suggestion to extend the
current loopclosure check to check more statements.

The current loopclosure flags patterns like:

	for k, v := range seq {
		go/defer func() {
			... k, v ...
		}()
	}

For this CL, the motivating example from golang/go#16520 is:

	var wg sync.WaitGroup
	for i := 0; i < 10; i++ {
		for j := 0; j < 1; j++ {
			wg.Add(1)
			go func() {
				fmt.Printf("%d ", i)
				wg.Done()
			}()
		}
	}
	wg.Wait()

The current loopclosure check does not flag this because of the
inner for loop, and the checker looks only at the last statement
in the outer loop body.

The suggestion is we redefine "last" recursively. For example,
if the last statement is an if, then we examine the last statements
in both of its branches. Or if the last statement is a nested loop,
then we examine the last statement of that loop's body, and so on.

A few years ago, Alan Donovan sent a sketch in CL 184537.

This CL attempts to complete Alan's sketch, as well as integrates
with the ensuing changes from golang/go#55972 to check
errgroup.Group.Go, which with this CL can now be recursively "last".

Updates golang/go#16520
Updates golang/go#55972
Fixes golang/go#30649
Fixes golang/go#32876

Change-Id: If66c6707025c20f32a2a781f6d11c4901f15742a
GitHub-Last-Rev: 04980e0
GitHub-Pull-Request: #415
Reviewed-on: https://go-review.googlesource.com/c/tools/+/452155
Reviewed-by: Tim King <taking@google.com>
Run-TryBot: Tim King <taking@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@findleyr findleyr

Labels
Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
5 participants
@rsc @timothy-king @dmitshur @gopherbot @findleyr