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: WaitGroup is still confusing #24010

Closed
anshulpundir opened this issue Feb 21, 2018 · 7 comments
Closed

sync: WaitGroup is still confusing #24010

anshulpundir opened this issue Feb 21, 2018 · 7 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@anshulpundir
Copy link

sync.WaitGroup is a synchronization primitive. But WaitGroup itself requires synchronization and linearization (of code execution), which defeats its purpose to some extent when using it to synchronize multiple go routines.

Additionally, the documentation should be made clearer to explicitly state that Add() and Wait() need to be synchronized using locks or called within the same execution stream.

What version of Go are you using (go version)?

go version go1.9.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

NA

What did you do?

Trying to use sync.WaitGroup in a "multi-threaded" application.

What did you expect to see?

sync.WaitGroup Add() and Wait() should not cause data races and work with concurrent usage without external synchronization.

What did you see instead?

sync.WaitGroup Add() and Wait() cause data races.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 21, 2018

This has already been documented more in the past. (bc64c07 for #8543)

I don't think more words will help at this point.

If one is trying to use WaitGroup with your own locking around it, that means they're either using it wrong, or haven't read the existing docs.

Can you give a concrete example of the type of code you're expecting to be easier?

Maybe you're looking for a different primitive.

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 21, 2018
@bradfitz bradfitz changed the title Make sync.WaitGroup more usable sync: WaitGroup is still confusing Feb 21, 2018
@anshulpundir
Copy link
Author

Thx for the quick response! @bradfitz

This has already been documented more in the past. (bc64c07 for #8543)

I did see that :)

If one is trying to use WaitGroup with your own locking around it, that means they're either using it wrong, or haven't read the existing docs.

Can you give a concrete example of the type of code you're expecting to be easier?

I'm trying to synchronize RPCs with shutting down of the server. The RPCs are not serviced from a common place so I can't guarantee the Add() before the Wait() gets called.

The behavior I'm looking for is for Add() and Wait() to be atomic and not race: the Add() calls that happen after Wait() should not affect the outcome of the Wait(). With my current use, Wait() races with Add() and causes a deadlock.

I've fallen back to using a RWMutex, which also not very elegant and seems odd to be used as a barrier during server shutdown.

@bradfitz
Copy link
Contributor

Yeah, I think WaitGroup is just not the tool you need.

This is probably best discussed on the mailing list where others can chime in. (i.e. this is more of a https://golang.org/wiki/Questions sort of thing)

I'll watch the lists and chime in if I have anything unique to add after hearing more details. The requirements are still a bit unclear for me at this point, but I do know that WaitGroup is not changing at this point.

@anshulpundir
Copy link
Author

anshulpundir commented Feb 21, 2018

Yeah, I think WaitGroup is just not the tool you need.

Can you please elaborate ? According to the docs: "A WaitGroup waits for a collection of goroutines to finish", which is basically what I'm trying to doing. @bradfitz

@anshulpundir
Copy link
Author

Also, I'm surprised that there is no well known pattern to synchronize server shutdown. I would assume that to be a frequent thing one has to deal with with golang.

The requirements are still a bit unclear for me at this point, but I do know that WaitGroup is not changing at this point.

The requirements are to make WaitGroup functions atomic, which they currently are not.

@cespare
Copy link
Contributor

cespare commented Feb 23, 2018

@anshulpundir as previously requested, please move this discussion to one of our mailing lists or another forum.

@anshulpundir
Copy link
Author

oops sorry! sure thing @cespare

@golang golang locked and limited conversation to collaborators Feb 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants