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

testing: t.Parallel in a subtest masks races with the outer test function #35670

Open
bcmills opened this issue Nov 18, 2019 · 2 comments
Open
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 18, 2019

The following test should reliably fail when run with -race, because it has multiple goroutines (via t.Parallel()) that all read from the (unsynchronized, changing) loop variable.

https://play.golang.org/p/nHo8MaTpS3o

func TestParallelRace(t *testing.T) {
	testCases := []struct {
		i int
	}{
		{0},
		{1},
		{2},
	}

	for _, tc := range testCases {
		t.Run(fmt.Sprint(tc.i), func(t *testing.T) {
			t.Parallel()
			t.Log(tc.i)
		})
	}
}

However, because t.Parallel blocks the goroutine until the main test function returns, the race goes undetected: as far as the race detector is concerned, the program unambiguously meant to use only the last value of the loop variable.

This unintended synchronization masks real bugs, such as the one reported in #35632.

#16520 requests a vet check that would detect this and similar cases, but perhaps there is something we can do within the implementation of t.Parallel in order to expose these races too.

$ go version
go version devel +2bde3c13 Mon Nov 18 05:26:46 2019 +0000 linux/amd64

CC @ianthehat @matloob @randall77

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 18, 2019
@bcmills bcmills added this to the Backlog milestone Nov 18, 2019
@bcmills bcmills changed the title testing,cmd/vet: t.Parallel masks capture of loop variables testing,cmd/vet: t.Parallel masks captured loop variables from the race detector Nov 18, 2019
@josharian
Copy link
Contributor

#18106 proposes doing something in cmd/vet specifically for t.Parallel. Given the puzzlement over how to fix #16520, it might be worth reconsidering the narrower problem.

@bcmills bcmills changed the title testing,cmd/vet: t.Parallel masks captured loop variables from the race detector testing: t.Parallel in a subtest masks races with the outer test function Nov 18, 2019
@bcmills
Copy link
Contributor Author

bcmills commented Nov 18, 2019

Hmm, good point. I don't actually think cmd/vet is the right tool here, since t.Parallel can also mask other kinds of races that would presumably not trigger the same vet warning. Retitled accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

2 participants