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: sync: add WaitGroup.Go method #39863

Open
oguzyildiz1991 opened this issue Jun 25, 2020 · 12 comments
Open

proposal: sync: add WaitGroup.Go method #39863

oguzyildiz1991 opened this issue Jun 25, 2020 · 12 comments
Labels
Projects
Milestone

Comments

@oguzyildiz1991
Copy link

@oguzyildiz1991 oguzyildiz1991 commented Jun 25, 2020

When I use errgroup.Group, I noticed I need much less code compared to sync.WaitGroup in certain situations. While keeping the existing methods of WaitGroup, can we add a Go(func()) function to make it more seamless? (And while at it, we could even make the Go method return the WaitGroup itself for chained calls? I wouldn't insist on this but it would be nice instead of it returning nothing)
It will be like:

wg := sync.WaitGroup{}
wg.Add(1)
go func() {
    defer wg.Done()
    doWork()  
}()

vs

wg := sync.WaitGroup{}.Go(func(){
    doWork()
})

or potentially:

wg := sync.WaitGroup{}.Go(doWork)

I feel like it's not a crazy far-fetched idea since errgroup.Group already has this method.

And this would be a nice addition for semaphore.Weighted as well, what do you think?

@gopherbot gopherbot added this to the Proposal milestone Jun 25, 2020
@gopherbot gopherbot added the Proposal label Jun 25, 2020
@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 25, 2020

Could you explain more about the use cases here. From the example you gave, you’re running a function in a goroutine and waiting for it to finish — that’s the same as calling the function directly.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 25, 2020

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 25, 2020
@oguzyildiz1991
Copy link
Author

@oguzyildiz1991 oguzyildiz1991 commented Jun 25, 2020

@davecheney of course that's not the whole usage :) I just didn't want to write more code than necessary to make it easier to read. I think it could apply any place you use WaitGroup where you're waiting other go routines to finish their computation.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 25, 2020

If you could give some examples that were closer to your intended usage that might help make your proposal more compelling.

@urandom
Copy link

@urandom urandom commented Jun 26, 2020

@davecheney
Perhaps the proposal is about aligning the WaitGroup API more towards ErrGroup's

wg := sync.WaitGroup{}
var urls = []string{
	"http://www.golang.org/",
	"http://www.google.com/",
	"http://www.somestupidname.com/",
}
for _, url := range urls {
	// Launch a goroutine to fetch the URL.
	url := url // https://golang.org/doc/faq#closures_and_goroutines
	wg.Go(func() {
		// Fetch the URL.
		resp, err := http.Get(url)
		if err == nil {
			resp.Body.Close()
		}
	})
}
// Wait for all HTTP fetches to complete.
if err := wg.Wait(); err == nil {
	fmt.Println("Successfully fetched all URLs.")
}
@bcmills
Copy link
Member

@bcmills bcmills commented Jun 26, 2020

And this would be a nice addition for semaphore.Weighted as well, what do you think?

I would rather add a semaphore to errgroup (#27837) than add goroutine-management to semaphore.Weighted.

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 26, 2020

I feel like it's not a crazy far-fetched idea since errgroup.Group already has this method.

I agree that this would be a reasonable addition to sync.WaitGroup, but note that the analogous API in errgroup.Group is much harder for callers to get right due to the error return value. (Using a context.WithContext and sync.WaitGroup directly, it's much too easy to accidentally cancel the Context too early and store a secondary error that occurs as a result, instead of the intended primary-cause error.)

Since sync.WaitGroup doesn't deal with errors or return values, there is less (but still non-trivial) risk of mismatched-Add bugs at the call site.

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 26, 2020

See previously #23538 and #18022.

@oguzyildiz1991
Copy link
Author

@oguzyildiz1991 oguzyildiz1991 commented Jun 26, 2020

@davecheney This is a very common pattern in our codebase, say I want to run 3 things in parallel. It'll be this:

wg := sync.WaitGroup{}
wg.Add(2)
go func() {
    defer wg.Done()
    doThis()  
}()
go func() {
    defer wg.Done()
    doThat()  
}()
doThose()
wg.Wait()

vs this:

wg := sync.WaitGroup{}
wg.Go(doThis)
wg.Go(doThat)
doThose()
wg.Wait()

This is also less bug prone for the cases when people call wrong amount of Add or Done

@networkimprov
Copy link

@networkimprov networkimprov commented Jun 26, 2020

I've used a similar pattern that passes the WaitGroup to ops which may run independently:

wg := sync.WaitGroup{}
wg.Add(1); go doThis(&wg)
wg.Add(1); go doThat(&wg)
doThose(nil)
wg.Wait()
@rsc rsc moved this from Incoming to Active in Proposals Jul 8, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jul 15, 2020

I can't find the previous discussion, although I'm sure there is one.
The general problem with this kind of method is that go, like defer, is carefully designed to give control over evaluating some parts of the call - namely the function being called and the arguments - in the original goroutine and only running the call itself in the new goroutine. Any Go method can't do that - it has to take a func(), with the pre-evaluation left to the caller (if the caller remembers!). That makes it a lot more clunky, and is why we've encouraged not wrapping the go statement and instead simply using it.

As for the chained ("fluent") calls, we've avoided that pattern in the Go standard library so far. It's just not idiomatic Go at this point. If you want to remember the receiver, use a variable.

If the problem is forgetting the wg.Add to match wg.Done, it still seems like a vet check would be best (#18022). I'm not sure we ever did that.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 22, 2020

Now I see that the previous discussion is #18022.
It still seems like we should try to figure out a good, accurate, no-false-positive way to check this in vet.

@rsc rsc changed the title proposal: errgroup.Group-like Go method for sync.WaitGroup proposal: sync: add WaitGroup.Go method Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.