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: x/sync: pass errgroup.WithContext's derived context directly #34510
Comments
@gopherbot , please add the proposal label |
The reason the g, ctx := errgroup.WithContext(ctx)
g.Go(func() error {
[…]
})
g.Go(func() error {
[…]
})
g.Go(func() error {
[…]
}) is a lot more concise than: g := errgroup.WithContext(ctx)
g.Go(func(ctx context.Context) error {
[…]
})
g.Go(func(ctx context.Context) error {
[…]
})
g.Go(func(ctx context.Context) error {
[…]
}) A lighter-weight closure syntax (#21498) would change the calculus on that. |
Indeed. That is why I added the call to |
Another option to consider would be something like this: package errgroup
type Group […]
func WithContext(ctx context.Context) (*Group, context.Context)
func (g *Group) Go(f func() error)
func (g *Group) Wait() error
func (g *Group) Context() context.Context // context.Background() if not created by WithContext That change would be backward-compatible, since it does not fundamentally alter the existing types or methods, but it's not fundamentally different from the user code presented for |
Add xsync.WorkGroup that is similar to https://godoc.org/golang.org/x/sync/errgroup, but amends it with a bit better (imho) design where work context is explicitly passed into worker (see also [1]). This mirrors sync.WorkGroup that we already have at Pygolang side [2]. [1] golang/go#34510 [2] https://pypi.org/project/pygolang/#concurrency
Point in case: xsync.WorkGroup (and the same functionality at Pygolang side: pyx | C++). |
Since API changes are something that is now possible to do with module versions, I thought it would be worth mentioning one that gnaws at me pretty frequently.
We use contexts heavily in my code-base, and it comes up moderately often that we want a cancellable or deadline-respecting
sync.WaitGroup
, and for this purpose authors tend to turn tox/sync/errgroup
. This often works out fine as long as the scope of theerrgroup.Group
is confined to a single function, but it also regularly will be used with something somewhat stateful. These frequently follow a certain pattern, the core elements of which are exemplified by this contrived example:I assert (without evidence) that this boilerplate is pretty common among context-respecting code that interacts with
errgroup.Group
, and in fact the above could be almost directly converted into a utility package, but then we would have ContextGroup wrapping errgroup wrapping WaitGroup... which feels excessive.I have also observed that the context returned by
WithContext
is occasionally misused for code other than the goroutines spawned byGo
, in some cases by directly shadowing thectx
variable, which often results in spooky action at a distance where one failure causes code in another part of the application to have its context cancelled.So, I propose that the API for errgroup split out the context- and non-context APIs:
Unfortunately, this is definitely a backward-incompatible change, and one for which there is probably little chance for a mechanical rewrite unless ContextGroup had a mechanism for retrieving its context.
The text was updated successfully, but these errors were encountered: