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

proposal: cmd/vet: add check for sync.WaitGroup abuse #18022

Open
dsnet opened this issue Nov 22, 2016 · 24 comments
Open

proposal: cmd/vet: add check for sync.WaitGroup abuse #18022

dsnet opened this issue Nov 22, 2016 · 24 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal-Hold
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Nov 22, 2016

The API for WaitGroup is very easy for people to misuse. The documentation for Add says:

calls to Add should execute before the statement creating the goroutine or other event to be waited for.

However, it is very common to see this incorrect pattern:

var wg sync.WaitGroup
defer wg.Wait()

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

This usage is fundamentally racy and probably does not do what the user wanted. Worse yet, is that it is not detectable by the race detector.

Since the above pattern is common, I propose that we add a method Go that essentially does the Add(1) and subsequent call to Done in the correct way. That is:

func (wg *WaitGroup) Go(f func()) {
	wg.Add(1)
	go func() {
		defer wg.Done()
		f()
	}()
}
@dsnet dsnet added the Proposal label Nov 22, 2016
@dsnet dsnet added this to the Proposal milestone Nov 22, 2016
@cznic
Copy link
Contributor

cznic commented Nov 23, 2016

I don't like the idea. It's IMHO perhaps a task for go vet, if not implemented already. Also, the proposed method would add a(nother) closure layer when the go-ed function has parameters.

@minux
Copy link
Member

minux commented Nov 23, 2016 via email

@dsnet
Copy link
Member Author

dsnet commented Nov 23, 2016

A go vet check seems pretty reasonable. I just tried it right now on the following:

func main() {
	var wg sync.WaitGroup
	defer wg.Wait()
	go func() {
		wg.Add(1)
		defer wg.Done()
	}()
}

and vet doesn't report anything.

In terms of vet's requirements:

  • Correctness: The problem is clearly a race.
  • Frequency: My gut feeling is that this is fairly common. A number of internal bugs that I've fixed is of this nature. So it subjective feels common enough. I don't have hard numbers.
  • Precision: I don't have an algorithm in mind that can accurately identify the pattern and I can imagine some number of false positives.

@dominikh , staticcheck does report a problem:
main.go:10:3: should call wg.Add(1) before starting the goroutine to avoid a race (SA2000)
I'm wondering how accurate the check is and whether it is worth adding to vet.

@mvdan
Copy link
Member

mvdan commented Nov 23, 2016

As far as the proposal goes, I don't like how it limits the func signature to func().

@dominikh
Copy link
Member

@dsnet The check in staticcheck has no (known) false positives. It shouldn't have a significant number of false negatives, either. The implementation is a simple pattern-based check, detecting go with function literals where the first statement is a call to wg.Add – this avoids flagging wg.Add calls further down the goroutine, which tend to be valid uses.

I'm -1 on the proposed Go function. I'd prefer not having to read code with an unnecessary level of nesting that looks callback-esque and reminds me of JavaScript.

@mvdan
Copy link
Member

mvdan commented Nov 23, 2016

@dominikh I don't see any extra level of nesting here, though (assuming any func signature is allowed).

To be nitpicky, another thing that stands out from the proposal is how wg.Go() will create a goroutine even though go is never directly used by the user. I don't know if the standard library does this anywhere else, but I would prefer if it was left explicit.

@dominikh
Copy link
Member

@mvdan The extra level of nesting would come from a predicted usage that looks something like this:

wg.Go(func() {
  // do stuff
})

as opposed to

go func() {
  // do stuff
}()

Admittedly the same level of indentation, but syntactically it's one extra level of nesting.

@mvdan
Copy link
Member

mvdan commented Nov 23, 2016

Ah yes, I was thinking indentation there.

@cznic
Copy link
Contributor

cznic commented Nov 23, 2016

The problematic case is that

wg.Add(1)
go func(i int) { ... }(42)

// becomes

wg.Go(func() {
        go func(i int) { ... }(42)
})

@mvdan
Copy link
Member

mvdan commented Nov 23, 2016

@cznic if you mean without the extra go, this would be solved if the restriction on the func() signature was removed.

@dominikh
Copy link
Member

@mvdan Do you mean by allowing something like the following?

wg.Go(func(x, y int) { ...}, v1, v2)

IMHO that's way too much interface{} and not enough type safety.

@mattn
Copy link
Member

mattn commented Nov 23, 2016

panic is recovered?

@mvdan
Copy link
Member

mvdan commented Nov 23, 2016

@dominikh true; I was simply pointing at the issue without contemplating a solution :)

@rsc
Copy link
Contributor

rsc commented Nov 28, 2016

The API change here has the problems identified above with argument evaluation. Also, in general the libraries do not typically phrase functionality in terms of callbacks. If we're going to start using callbacks broadly, that should be a separate decision (and not one to make today). For both these reasons, it seems like .Go is not a clear win.

It would be nice to have a vet check that we trust (no false positives). Perhaps it is enough to look for two statements

wg.Add(1)
defer wg.Done()

back to back and reject that always. Thoughts about how to make vet catch this reliably?

@renannprado
Copy link

I agree that vet is better place for this. The proposed API reminds of JavaScript, which will force us many times to wrap the code or function within a function with no arguments, while you could have just go func()....
Still nothing can stop you from creating such a helper methid, even though I don't see the need for it.

@rsc
Copy link
Contributor

rsc commented Dec 5, 2016

It sounds like we are deciding to make go vet check this and not add new API here. Any arguments against that?

@dsnet
Copy link
Member Author

dsnet commented Dec 5, 2016

SGTM

@dsnet dsnet changed the title proposal: sync: add Go method to WaitGroup cmd/vet: add check for sync.WaitGroup abuse Dec 5, 2016
@dsnet dsnet removed the Proposal label Dec 9, 2016
@rsc rsc added this to Active in Proposals (old) Aug 5, 2020
@rsc
Copy link
Contributor

rsc commented Aug 5, 2020

I've added this proposal to the proposal process bin, but it's blocked on someone figuring out how to implement a useful check. Is anyone interested in doing that?

@dominikh
Copy link
Member

dominikh commented Aug 6, 2020

Staticcheck has a fairly trivial check: for a GoStmt of a FuncLit, if the first statement in the FuncLit is a call to (*sync.WaitGroup).Add, we flag it. That has potential for false positives, but none have been reported in all the years that the check has existed.

The check could be trivially hardened by

  1. looking for an immediately following defer of (*sync.WaitGroup).Done and comparing the two receivers.
  2. checking that the argument to Add is 1, not some other number.

Edit: which is pretty much what you have suggested in #18022 (comment)

@rsc
Copy link
Contributor

rsc commented Aug 12, 2020

Thanks for the info @dominikh.
Would it be OK with you for vet to do the same thing?
Would you want to send the code?

@rsc
Copy link
Contributor

rsc commented Aug 26, 2020

Ping @dominikh. Thanks.

@dominikh
Copy link
Member

Sorry @rsc, I missed your earlier comment. Sure, that's fine by me. I can send code if it helps, but I'd have to write it from scratch – the check in staticcheck makes use of staticcheck internals.

@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

@dominikh sure, if you feel like sending code please do. Otherwise anyone else who wants to is welcome. I just want to make sure we're not overstepping versus staticcheck (which I'm a big fan of).

@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

On hold for details of the eventual implemented check.

@rsc rsc moved this from Active to Hold in Proposals (old) Sep 2, 2020
@rsc rsc changed the title cmd/vet: add check for sync.WaitGroup abuse proposal: cmd/vet: add check for sync.WaitGroup abuse Sep 2, 2020
dsnet added a commit to tailscale/tailscale that referenced this issue 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.

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
dsnet added a commit to tailscale/tailscale that referenced this issue 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.

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
dsnet added a commit to tailscale/tailscale that referenced this issue Mar 9, 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>
darksip pushed a commit to darksip/tailscale that referenced this issue 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>
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal-Hold
Projects
Status: Hold
Development

No branches or pull requests

9 participants