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

sync: clarify WaitGroup.Done() behaviour when the counter is zero #39679

Open
leventov opened this issue Jun 18, 2020 · 7 comments
Open

sync: clarify WaitGroup.Done() behaviour when the counter is zero #39679

leventov opened this issue Jun 18, 2020 · 7 comments

Comments

@leventov
Copy link

@leventov leventov commented Jun 18, 2020

I think it's worth noting that WaitGroup.Done() panics if the counter is already zero. It's currently inferrable from the docs, but not super obvious.

@andybons andybons changed the title Clarify WaitGroup.Done() behaviour when the counter is zero. sync: clarify WaitGroup.Done() behaviour when the counter is zero Jun 18, 2020
@andybons andybons added this to the Unplanned milestone Jun 18, 2020
@andybons
Copy link
Member

@andybons andybons commented Jun 18, 2020

What do you think, @bcmills?

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 18, 2020

A panic in Go is a recoverable operation, but a surplus call to Done is often accompanied by a data race (which is undefined behavior, not a recoverable error). So I think it would be a mistake to define a surplus call to Done as a panic, since the panic is only defined behavior for a narrow subset of surplus-Done calls.

On the other hand, we have already made that mistake for the Add method, and Done() is equivalent to Add(-1).

@leventov
Copy link
Author

@leventov leventov commented Jun 18, 2020

Could there be any data race on the WaitGroup's memory as a result of calling public WaitGroup's methods? If not, and there is just a speculation that a surplus call to Done is accompanied by some data race somewhere around WaitGroup, but not WaitGroup itself, I find this is a strange justification for not describing what will happen if Done() is called several times. Similar argument could be made about any function: there could be a data race before the call to it.

Developers with Java background where extra CountDownLatch.countDown() calls are no-op, could call Done() mindlessly, e. g. in a loop, thinking an extra guard boolean variable is unnecessary.

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 18, 2020

@leventov, if the counter goes negative, an overflow occurs in the WaitGroup (#20687), which can corrupt the waiter count:
https://github.com/golang/go/blob/go1.14.4/src/sync/waitgroup.go#L64

That is not formally a data race, but it's also not a recoverable condition. You really don't want to call Done mindlessly in a loop, even if you recover the resulting panic.

@tkleczek
Copy link

@tkleczek tkleczek commented Nov 11, 2020

Recently, not being aware that Done panics if counter goes negative I wrote the following code (basically I wanted to ublock main as soon as any of the goroutines exit)
https://play.golang.org/p/3f_9iOk7GEY

If the docs were clearer on Done behavior, this would not have happened

@go101
Copy link

@go101 go101 commented Nov 12, 2020

Done is a shortcut of Add(-1). The behavior is explained in Add docs.

Maybe the docs of Done should mention this fact (it is a shortcut of Add(-1)).

@tkleczek
Copy link

@tkleczek tkleczek commented Nov 12, 2020

Maybe the docs of Done should mention this fact (it is a shortcut of Add(-1)).

@go101 Yes, if this was mentioned in the docs, sure. But otherwise it's an implementation detail that I have to browse the code to learn. (and don't know if I can rely on this from release to release)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants