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: x/sync: pass errgroup.WithContext's derived context directly #34510

Open
kylelemons opened this issue Sep 24, 2019 · 5 comments
Open

Comments

@kylelemons
Copy link
Contributor

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 to x/sync/errgroup. This often works out fine as long as the scope of the errgroup.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:

type ServerRunner struct {
  group   *errgroup.Group // *Group as a field
  groupCtx context.Context // context as a field (heavily discouraged)
}

func (sr *ServiceRunner) Run(server *servers.Server) {
  sr.Group.Go(func() error { return server.Run(sr.groupCtx) }) // wrapper that calls Go
}

func (sr *ServiceRunner) Wait(ctx context.Context) {
  select {
  case <-ctx.Done():
    return ctx.Err()
  case <-sr.groupCtx.Done():
    sr.groupCtx.Wait() // wrapper that calls wait and/or ctx.Done
    return sr.groupCtx.Err()
  }
}

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 by Go, in some cases by directly shadowing the ctx 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:

type Group struct { /* ... */ }
func (Group) Go(func() error) { /* ... */ }
func (Group) Wait() error { /* ... */ }

type ContextGroup struct { /* ... */ }
func WithContext(ctx context.Context) *ContextGroup { /* ... */ }
func (ContextGroup) Go(func(context.Context) error) { /* ... */ }
func (ContextGroup) Wait(context.Context) error { /* ... */ }

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.

@gopherbot gopherbot added this to the Unreleased milestone Sep 24, 2019
@kylelemons
Copy link
Contributor Author

@gopherbot , please add the proposal label

@agnivade agnivade changed the title x/sync proposal: pass errgroup.WithContext's derived context directly proposal: x/sync: pass errgroup.WithContext's derived context directly Sep 25, 2019
@bcmills
Copy link
Contributor

bcmills commented Oct 8, 2019

The reason the Context parameter is omitted today is primarily that it adds a lot of boilerplate:

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.

@bcmills
Copy link
Contributor

bcmills commented Oct 8, 2019

I have also observed that the context returned by WithContext is occasionally misused for code other than the goroutines spawned by Go, in some cases by directly shadowing the ctx variable

Indeed. That is why I added the call to cancel() in Wait(), although that admittedly does not catch all such cases.

@bcmills
Copy link
Contributor

bcmills commented Oct 8, 2019

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 ServerRunner.

navytux added a commit to navytux/go123 that referenced this issue Jan 13, 2020
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
@navytux
Copy link
Contributor

navytux commented Jan 13, 2020

Point in case: xsync.WorkGroup (and the same functionality at Pygolang side: pyx | C++).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants