-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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 golang.org/x/sync/errgroup #57534
Comments
Note that |
Note that |
Having a function just named "WithContext" in package sync seems not great, since it would be called like |
That's a good point. However, naming can be revised in other liked forms, the core idea is to include |
Personally I don't think the low-level sync package is the right place for the high level errgroup concept. Let's not assume that x/sync has to match to the standard library sync. |
I think this is a common enough need and has proven its worth. Most users shouldn’t use the original WaitGroup anymore. My only question would be if there should be a sync/v2 with this plus generics for Map etc. |
@ianlancetaylor I am also fine if it is a sub package of sync The primary purpose is to get it into stdlib, and the naming can be decided into whatever community likes. If the decision is to have a standalone package, an emphasis comment in the WaitGroup can increase the public exposure of errgroup. |
With the addition of I don't think it must necessarily use |
I had been joining errors in my errutil.ExecParallel helper, which uses my pre-Go 1.20 multierror type. I'm not sure though if it would work as well for errgroup. |
Hi, original There are two significant problems with the API:
return err instead of cancel()
g.Wait()
return err Those can both be remedied by adding a method g, ctx := errgroup.New(ctx)
defer g.Stop()
…
if err != nil {
return err
} Ideally, that would be accompanied by an improvement to the |
@apparentlymart, I do not think it would be appropriate for |
Also, even on error and canceled context it still creates remaining goroutines. Consider this example with 10k batch. errg, ctx := errgroup.WithContext(context.Background())
items := []int{1, 2, 3, ...... 10000}
for _, item := range items {
item := item
errg.Go(func() error {
if err := process(ctx, item); err != nil {
return fmt.Errorf("something went wrong while processing item %d", item)
}
return nil
})
}
err := errg.Wait() Usually simple fix would be to check for canceled context. But this doesn't always look obvious. errg, ctx := errgroup.WithContext(context.Background())
items := []int{}
loop:
for _, item := range items {
item := item
select {
case <-ctx.Done():
break loop
default:
}
errg.Go(func() error {
if err := process(ctx, item); err != nil {
return fmt.Errorf("something went wrong while processing item %d", item)
}
return nil
})
}
err := errg.Wait() @bcmills Do you have thoughts on how can api look to bake this cancellation into errgroup package? |
Hi @anjmao if I'm understanding right what you are proposing, it's the same as https://pkg.go.dev/github.com/sourcegraph/conc/pool#ContextPool.WithCancelOnError. Maybe conc's API can be used as an inspiration for this.
|
@jimen0 errgroup package already works like that and cancels context when created using WithContext. The issue that even if your context is canceled errgroup.Go will spawn new goroutine and run your function. |
I agree, |
Re: @bcmills
That is only true if you construct the errgroup using To thread that needle, perhaps it could return |
That's an interesting suggestion, but I'm not sure how well it would work out in practice — errors that stem from a Context cancellation don't necessarily wrap For example, a function that performs a network operation might use |
@bcmills good point. Strictly speaking, if you returned a composite, ordered error, anyone who wanted could pick out just the first error, but that's a common enough use case that it'd be annoying to have to do the dance. Yet another perspective is that Wait could return the first error when there is a cancelable context set up, but an errors.Join of all errors when there isn't. That should track the likely meaningfulness of the errors. |
That, and worse: if someone has a very long-lived
That would be possible, but it seems a little too magical to me. If we're going down this road, I would rather have an explicit call or parameter to toggle the behavior. |
Works for me. It'd be much nicer than my current code that separately tracks and juggles a slice of errors... |
The cleanest alternative right now is a errs := make(chan []error, 1)
errs <- nil
saveErr := func(p *error) {
if *p != nil {
errs <- append(<-errs, *p)
}
}
g.Go(func() (err error) {
defer saveErr(&err)
…
}) |
My own
@bcmills wrote:
That's the reason for my last feature. |
I often find myself using an for e := range events {
if !group.TryGo(task) {
// Here I need to know that the task couldn't be started
// I might e.g. need a cancellation handle for ad-hoc task spawning
}
} In another goroutine: <-ctx.Done()
group.Wait()
// All tasks should be complete. With current g, ctx := errgroup.WithContext(context.Background)
g.Wait() // Done, right?
g.TryGo(fn) // Reports true and runs the goroutine, after it's waited. Risk of leaks So I'm very much in favor of a
As an aside, would it be wise to break out and remove the semaphore features ( |
My 2 cents: the problem of managing a long-running pool of workers seems very different to me than what |
I'm for it. Also (not sure it's very relevant information): this proposal was just mentioned by Yarden Laifenfeld in GopherCon Israel and the vibe in the room was of general consensus to add this to |
I don't think One gripe I have with it is the How often do people change the limit of an existing The only remaining question is what the factory function(s) for groups would look like. I don't have a satisfying answer, but I've been experimenting with the following API: package errgroup
type Group struct {
// contains filtered or unexported fields
}
func New(opts ...Option) (*Group, error)
func (g *Group) Context() context.Context
func (g *Group) Go(f func() error)
func (g *Group) TryGo(f func() error) bool
func (g *Group) Wait() error
type Option interface {
// contains filtered or unexported methods
}
func WithContext(ctx context.Context) Option
func WithLimit(n int) Option I know that many members of the community (@bcmills included) don't like functional options, and I've certainly softened my stance about them since I gave a talk about the pattern at GopherCon Europe 2023, but if you can stomach them, client code is quite nice: g, err := errgroup.New(
errgroup.WithContext(ctx),
errgroup.WithLimit(64),
)
if err != nil {
// handle error
} In particular, a single factory function would be enough and its One main downside is that it allocates more than the current API. Anyway, I'm not seriously suggesting that the hypothetical new API should look like this; I just wanted to share my thoughts here. |
Did you mean to include |
FWIW, I wrote it on some other issue, but I really like https://pkg.go.dev/cmd/internal/par and wish it were exported. It's a very slick and useful API. You could add errgroup to it and that would be a fairly complete set of high level tools for concurrency management. |
Maybe worth noting that https://pkg.go.dev/github.com/rogpeppe/go-internal/par is available, though I think it is an earlier snapshot of the cmd/go internal package (including it has less functionality than what is currently used in cmd/go). |
It's out of date because it's from before generics. |
You're correct. My mistake. Fixed. Thanks!
That package wasn't on my radar! I'll check it out. |
I'm realising now that a context-aware If anything, this subtlety reinforces my belief that the API would be improved by the removal of the |
Another idea for the API is to move context into the callback: package errgroup
type Group struct {
// contains filtered or unexported fields
}
func New(opts ...Option) (*Group, error)
func (g *Group) Go(f func(context.Context) error)
func (g *Group) TryGo(f func(context.Context) error) bool
func (g *Group) Wait() error
type Option interface {
// contains filtered or unexported methods
}
func WithContext(ctx context.Context) Option
func WithLimit(n int) Option That makes the scope of the derived context pretty clear. Having two options leaves open the door to more, but will this really develop more options? With the package par // import "sync/par"
type Group struct {
// contains filtered or unexported fields
}
func NewGroup(context.Context, int) (*Group, error)
func (g *Group) Go(f func(context.Context) error)
func (g *Group) TryGo(f func(context.Context) error) bool
func (g *Group) Wait() error |
I like the idea! I think that's one improvement that Peter Bourgon hinted at years ago in one of his talks.
I could live without functional options.
I would expect By the way, would your factory function even need to return an |
errgroup.Group behaves differently if it has a context or not. If we wanted to lose the behavior of not cancelling that comes from not having a context, then we could require the context to be non-nil. |
I'd be in favour of requiring a non-nil context, |
If errgroup's API is augmented with a method for returning all the errors, that method should return, not a |
I'm not sure. If the group has to store the errors in a slice anyways, I don't really see the harm of returning a slice (note that an I'd want to see a fuller API design that addresses these.
Just to mention an alternative: We have However, I generally like the idea of passing the I don't know what I prefer, at the end of the day. But I don't really find any of the options presented so far clearly right. |
I remain opposed to a method to get all of the errors: that would make the storage requirement of an In typical |
As briefly discussed in #56102 (comment), I propose to promote
errgroup.Group
to thesync
package. The proposed API set is listed below.Rationale
Compared to
sync.WaitGroup
,errgroup.Group
do not require cognitive load to manageAdd()
andDone()
and can easily manage the number of concurrent tasks usingSetLimit
. For example,vs.
Tu et al. [1] also reported that
WaitGroup
is often misused and causes concurrency bugs. For instance, an example taken from Figure 9:[1] Tu, Tengfei, et al. "Understanding real-world concurrency bugs in go." Proceedings of the Twenty-Fourth International Conference on Architectural Support for Programming Languages and Operating Systems. 2019. https://doi.org/10.1145/3297858.3304069
Existing Usage
A search over GitHub, code that uses
errgroup.Group
includes 16.1k files and pkg.go.dev shows there are 10,456 imports.APIs
Update:
WithContext
is renamed toNewErrGroupWithContext
.The text was updated successfully, but these errors were encountered: