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/errgroup: improve api for context values #39312
Comments
Please provide concrete examples. I suspect that you will find that calls to
The zero |
An example from appleboy/gorush; var g errgroup.Group
// Run httpd server
g.Go(func() error {
return gorush.RunHTTPServer(ctx)
})
// Run gRPC internal server
g.Go(func() error {
return rpc.RunGRPCServer(ctx)
})
// check job completely
g.Go(func() error {
select {
case <-finished:
}
return nil
})
if err = g.Wait(); err != nil {
gorush.LogError.Fatal(err)
} After: // we already had "ctx" which parent propagates
// it's Done() to the child.
g, ctx := errgroup.WithContext(ctx)
// Run httpd server
g.GoContext(gorush.RunHTTPServer)
// Run gRPC internal server
g.GoContext(rpc.RunGRPCServer)
// check job completely
g.Go(func() error {
select {
case <-finished:
}
return nil
})
if err = g.Wait(); err != nil {
gorush.LogError.Fatal(err)
} Now, the code is more succint, doesn't wrap the functions with already expected function signatures, and the comments before individual // check job completely
g.GoContext(func(ctx context.Context) error {
select {
case <-finished:
case <-ctx.Done():
}
return nil
}) This also solves a bug directly related to the group cancellation. As soon as any of the other functions exit with an error, the last function has the opportunity to return, allowing the error to bubble up the stack and get handled. If to take a look at net/http.Request as I suggested, Context() should return // NewRequest wraps NewRequestWithContext using the background context.
func NewRequest(method, url string, body io.Reader) (*Request, error) {
return NewRequestWithContext(context.Background(), method, url, body)
} So, the func (g *Group) Context() context.Context {
if g.ctx == nil {
return context.Background()
}
return g.ctx
}
That's exactly the argument for adding a context-aware API. Even with the example above it's easy to see how not having a cancellation context from the group can lead to serious problems. If anything, passing context explicitly should be preferred over relying on scope, that's the main argument for a GoContext(ctx) addition to the API. |
@titpetric, that example seems pretty convoluted to begin with.
I think as motivating examples go, that one needs some concurrency cleanup before we can use it to conclude anything about |
Thinking about this some more, it might be feasible to use a That would likely provide a better default than However, |
As a side-bar, what would be your thoughts on satisfying a context.Context value with errgroup.Group?
I'm also not completely sold on my GoContext() suggestion. What I mean by that is that maybe, if a breaking change would ever be OK here, it could be decided just to take |
|
This proposal has been declined as retracted. |
I propose extending the x/errgroup API with a few convenience functions,
This requires:
The convenience function can be easily implemented as a wrapper:
The suggested naming is in line with
http.Request
Context(), anddatabase/sql
function signatures like ExecContext(...). The impact on code readability on codebases should be positive, as the wrapping for context-aware functions is eliminated on the consumer implementation side. There are no backwards compatibility issues that I can see.Are PRs for this area accepted, any concerns? I'm more than willing to submit a PR for extending the functionality of this package.
The text was updated successfully, but these errors were encountered: