Fix incorrect use of loop variables in parallel tests. #44013
Conversation
This commit fixes occurrences of loop variables being captured by reference in parallel tests. With very high probability, only the last test case will actually be exercised. To work around this problem, we create a local copy of the range variable before the parallel test, as advised by the Go documentation at: https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks Issues were found automatically using the `loopvarcapture` linter. Signed-off-by: Renato Costa <renato@cockroachlabs.com>
| tailFiles(files, watcher, dec, tailReader, config.Tail, fwd) | ||
| close(started) |
There was a problem hiding this comment.
The synchronization attempted in this loop was incorrect (closing the channel in the Go routine and reading it back, supposedly to wait for the Go routine to finish). Since close was called before the actual reference to the range variable, the code was not safe.
Maybe this snippet makes it clearer: https://go.dev/play/p/mo3FA7_D8sI
There was a problem hiding this comment.
I think it was meant to wait for the goroutine to start, not finish (hence the name started).
Not sure if it makes any difference though.
There was a problem hiding this comment.
I must admit that I didn't look closely at this part. Is there something to dereference / capture in this case as well then? The playground example above (https://go.dev/play/p/mo3FA7_D8sI) shows;
`close` called at the beginning, tc = 3
`close` called at the beginning, tc = 3
`close` called at the beginning, tc = 3
`close` called at the end, tc = 1
`close` called at the end, tc = 2
`close` called at the end, tc = 3
Which, likely isn't intended.
Changing the example to capture tc https://go.dev/play/p/aZ7YsyPFxMN shows;
`close` called at the beginning, tc = 3
`close` called at the beginning, tc = 2
`close` called at the beginning, tc = 1
`close` called at the end, tc = 1
`close` called at the end, tc = 2
`close` called at the end, tc = 3
There was a problem hiding this comment.
I think it was meant to wait for the goroutine to start, not finish (hence the name started).
Good point. I admit I just made the minimal change here to make the loop variable access safe.
To your point, @thaJeztah, I could just create a copy of config, as that would also fix the issue. On the other hand, it seems like this is dead code, as the iteration happens over an empty map.
|
Superseded by #45005 |
- What I did
This commit fixes occurrences of loop variables being captured by
reference in parallel tests. With very high probability, only the last
test case will actually be exercised. To work around this problem, we
create a local copy of the range variable before the parallel test, as
advised by the Go documentation at:
https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks
- How I did it
Issues were found automatically using the
loopvarcapturelinter.- How to verify it
Running the tests changed here and verifying that they now actually run each test declared.