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

Closed
oguzyildizz opened this issue Jun 25, 2020 · 15 comments
Closed

proposal: sync: add WaitGroup.Go method #39863

oguzyildizz opened this issue Jun 25, 2020 · 15 comments

Comments

@oguzyildizz
Copy link

oguzyildizz 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
@davecheney
Copy link
Contributor

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

CC @bcmills

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jun 25, 2020
@oguzyildizz
Copy link
Author

@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

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 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
Contributor

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
Contributor

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
Contributor

bcmills commented Jun 26, 2020

See previously #23538 and #18022.

@oguzyildizz
Copy link
Author

oguzyildizz 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

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 (old) Jul 8, 2020
@rsc
Copy link
Contributor

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 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
@rsc
Copy link
Contributor

rsc commented Aug 5, 2020

Based on the discussion above, I suggest we decline this issue and continue discussion of the vet check on the previously-filed #18022. Does anyone object to that?

@rsc
Copy link
Contributor

rsc commented Aug 12, 2020

Based on the discussion above, this seems like a likely decline.

@rsc
Copy link
Contributor

rsc commented Aug 26, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Aug 26, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Aug 26, 2020
@golang golang locked and limited conversation to collaborators Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

8 participants