Skip to content

Conversation

spikecurtis
Copy link
Contributor

@spikecurtis spikecurtis commented Aug 28, 2025

Fixes: coder/internal#950

Pretty sure the intention of the hold wait group is to try to get the two goroutines that the test starts running at the same time. But, that should be the case for two goroutines started anyway.

The use of hold doesn't actually guarantee concurrent execution of Acquire, just that both goroutines get as far as Done() --- the go scheduler could run them serially without incident.

So I've chosen to just remove the use of hold to simplify.

But, for posterity, the data race was due to incrementing by 1 in the loop along with the goroutine that calls Done. You could increment by 1 and then back down to 0 before the second iteration of the loop starts. This then causes a data race with calling Wait() in the first goroutine and Add() in the second iteration. c.f. https://pkg.go.dev/sync#WaitGroup.Add

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@spikecurtis spikecurtis requested a review from aslilac August 28, 2025 13:28
@spikecurtis spikecurtis marked this pull request as ready for review August 28, 2025 13:28
@spikecurtis spikecurtis force-pushed the spike/internal-950-data-race branch from eb6d892 to c40cee0 Compare August 28, 2025 13:29
Copy link
Member

@aslilac aslilac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for waiting for me to get back from vacation for the review 😄

@spikecurtis spikecurtis merged commit ee35ad3 into main Sep 3, 2025
29 checks passed
@spikecurtis spikecurtis deleted the spike/internal-950-data-race branch September 3, 2025 05:25
@github-actions github-actions bot locked and limited conversation to collaborators Sep 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flake: data race in TestConcurrentFetch

2 participants