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
Milestone

Comments

@bcmills
Copy link
Member

@bcmills 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 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

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Nov 18, 2019

#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

This comment has been minimized.

Copy link
Member Author

@bcmills 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
Projects
None yet
2 participants
You can’t perform that action at this time.