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

syncs: add WaitGroup wrapper #7481

Merged
merged 1 commit into from Mar 9, 2023
Merged

syncs: add WaitGroup wrapper #7481

merged 1 commit into from Mar 9, 2023

Conversation

dsnet
Copy link
Member

@dsnet dsnet commented Mar 7, 2023

The addition of WaitGroup.Go in the standard library has been repeatedly proposed and rejected.
See golang/go#18022, golang/go#23538, and golang/go#39863

In summary, the argument for WaitGroup.Go is that it avoids bugs like:

go func() {
	wg.Add(1)
	defer wg.Done()
	...
}()

where the increment happens after execution (not before) and also (to a lesser degree) because:

wg.Go(func() {
	...
})

is shorter and more readble.

The argument against WaitGroup.Go is that the provided function takes no arguments and so inputs and outputs must closed over by the provided function. The most common race bug for goroutines is that the caller forgot to capture the loop iteration variable, so this pattern may make it easier to be accidentally racy. However, that is changing with golang/go#57969.

In my experience the probability of race bugs due to the former still outwighs the latter, but I have no concrete evidence to prove it.

The existence of errgroup.Group.Go and frequent utility of the method at least proves that this is a workable pattern and the possibility of accidental races do not appear to manifest as frequently as feared.

A reason not to use errgroup.Group everywhere is that there are many situations where it doesn't make sense for the goroutine to return an error since the error is handled in a different mechanism (e.g., logged and ignored, formatted and printed to the frontend, etc.). While you can use errgroup.Group by always returning nil, the fact that you can return nil makes it easy to accidentally return an error when nothing is checking the return of group.Wait. This is not a hypothetical problem, but something that has bitten us in usages that was only using errgroup.Group without intending to use the error reporting part of it.

Thus, add a (yet another) variant of WaitGroup here that is identical to sync.WaitGroup, but with an extra method.

syncs/syncs.go Show resolved Hide resolved
The addition of WaitGroup.Go in the standard library has been
repeatedly proposed and rejected.
See golang/go#18022, golang/go#23538, and golang/go#39863

In summary, the argument for WaitGroup.Go is that it avoids bugs like:

	go func() {
		wg.Add(1)
		defer wg.Done()
		...
	}()

where the increment happens after execution (not before)
and also (to a lesser degree) because:

	wg.Go(func() {
		...
	})

is shorter and more readble.

The argument against WaitGroup.Go is that the provided function
takes no arguments and so inputs and outputs must closed over
by the provided function. The most common race bug for goroutines
is that the caller forgot to capture the loop iteration variable,
so this pattern may make it easier to be accidentally racy.
However, that is changing with golang/go#57969.

In my experience the probability of race bugs due to the former
still outwighs the latter, but I have no concrete evidence to prove it.

The existence of errgroup.Group.Go and frequent utility of the method
at least proves that this is a workable pattern and
the possibility of accidental races do not appear to
manifest as frequently as feared.

A reason *not* to use errgroup.Group everywhere is that there are many
situations where it doesn't make sense for the goroutine to return an error
since the error is handled in a different mechanism
(e.g., logged and ignored, formatted and printed to the frontend, etc.).
While you can use errgroup.Group by always returning nil,
the fact that you *can* return nil makes it easy to accidentally return
an error when nothing is checking the return of group.Wait.
This is not a hypothetical problem, but something that has bitten us
in usages that was only using errgroup.Group without intending to use
the error reporting part of it.

Thus, add a (yet another) variant of WaitGroup here that
is identical to sync.WaitGroup, but with an extra method.

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
@dsnet dsnet merged commit dad78f3 into main Mar 9, 2023
33 checks passed
@dsnet dsnet deleted the dsnet/syncs-group branch March 9, 2023 20:04
darksip pushed a commit to darksip/tailscale that referenced this pull request Apr 3, 2023
The addition of WaitGroup.Go in the standard library has been
repeatedly proposed and rejected.
See golang/go#18022, golang/go#23538, and golang/go#39863

In summary, the argument for WaitGroup.Go is that it avoids bugs like:

	go func() {
		wg.Add(1)
		defer wg.Done()
		...
	}()

where the increment happens after execution (not before)
and also (to a lesser degree) because:

	wg.Go(func() {
		...
	})

is shorter and more readble.

The argument against WaitGroup.Go is that the provided function
takes no arguments and so inputs and outputs must closed over
by the provided function. The most common race bug for goroutines
is that the caller forgot to capture the loop iteration variable,
so this pattern may make it easier to be accidentally racy.
However, that is changing with golang/go#57969.

In my experience the probability of race bugs due to the former
still outwighs the latter, but I have no concrete evidence to prove it.

The existence of errgroup.Group.Go and frequent utility of the method
at least proves that this is a workable pattern and
the possibility of accidental races do not appear to
manifest as frequently as feared.

A reason *not* to use errgroup.Group everywhere is that there are many
situations where it doesn't make sense for the goroutine to return an error
since the error is handled in a different mechanism
(e.g., logged and ignored, formatted and printed to the frontend, etc.).
While you can use errgroup.Group by always returning nil,
the fact that you *can* return nil makes it easy to accidentally return
an error when nothing is checking the return of group.Wait.
This is not a hypothetical problem, but something that has bitten us
in usages that was only using errgroup.Group without intending to use
the error reporting part of it.

Thus, add a (yet another) variant of WaitGroup here that
is identical to sync.WaitGroup, but with an extra method.

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants