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/errgroup: improve api for context values #39312

Open
titpetric opened this issue May 29, 2020 · 6 comments
Open

proposal: x/sync/errgroup: improve api for context values #39312

titpetric opened this issue May 29, 2020 · 6 comments
Labels
Projects
Milestone

Comments

@titpetric
Copy link

@titpetric titpetric commented May 29, 2020

I propose extending the x/errgroup API with a few convenience functions,

  • Context() context.Context - return the error group cancelable context,
  • GoContext(func(context.Context) error) - pass the produced ctx on to functions,

This requires:

  1. Add the produced context.Context value to Group{},
  2. Have the ability to read it afterwards with Context(),
  3. Pass context to functions with GoContext(func(context.Context) error) convenience

The convenience function can be easily implemented as a wrapper:

func (g *Group) GoContext(fn func(context.Context) error) {
        g.Go(func() error {
                return fn(g.Context())
        }
}

The suggested naming is in line with http.Request Context(), and database/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.

@gopherbot gopherbot added this to the Unreleased milestone May 29, 2020
@andybons andybons changed the title x/errgroup: improve api for context values proposal: x/sync/errgroup: improve api for context values May 29, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented May 29, 2020

The impact on code readability on codebases should be positive, as the wrapping for context-aware functions is eliminated on the consumer implementation side.

Please provide concrete examples. I suspect that you will find that calls to GoContext are in practice much more verbose than closing over the context with the current API.

There are no backwards compatibility issues that I can see.

The zero errgroup.Group today is valid, and collects errors without cancelling anything on error. What should its Context method return, and what Context should it pass to GoContext callbacks?

Loading

@titpetric
Copy link
Author

@titpetric titpetric commented May 29, 2020

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 Go*() calls feel redundant. Depending on what the code outside of the example does to trigger the error, it's also possible that the last function won't finish, meaning Wait() will never exit. We can fix that:

	// 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 context.Background() when it's internal context is a nil value. An zero context can be checked against nil.

// 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 Context() context.Context would be implemented as follows:

func (g *Group) Context() context.Context {
        if g.ctx == nil {
                return context.Background()
        }
        return g.ctx
}

The zero errgroup.Group today is valid, and collects errors without cancelling anything on error.

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.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented May 29, 2020

@titpetric, that example seems pretty convoluted to begin with.

  • Why is the goroutine executing close(finished) not started within the errgroup in the first place?
  • Why does withContextFunc accept a callback, rather than returning a context that will be canceled appropriately? (That would make the function much easier to structure as a more typical g.Go callback.)
  • Given that InitWorkers accepts a context.Context, why does it also need to be passed a sync.WaitGroup (rather than blocking until the Context is done, then shutting down workers synchronously before returning)?

I think as motivating examples go, that one needs some concurrency cleanup before we can use it to conclude anything about errgroup API improvements.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 1, 2020

Thinking about this some more, it might be feasible to use a sync.Once to allow the zero Group to initialize its own context.WithCancel the first time the Context method is called.

That would likely provide a better default than context.Background(), since it would allow the zero Group to support the same cancel-on-error behavior as other groups.

However, context.WithCancel requires that the cancel function be called, so we might need some other API in order to avoid leaking resources. (In CL 134395, that API is a Stop method, but there would still be a potential leak if a caller that invokes Context is paired with an an older caller that allocates a zero Group and doesn't know about the Stop method.)

Loading

@titpetric
Copy link
Author

@titpetric titpetric commented Jun 8, 2020

As a side-bar, what would be your thoughts on satisfying a context.Context value with errgroup.Group?

  • The WithContext() API becomes problematic with such a change,
    • would WithContext use return wg, wg?
    • should there be a NewGroup(ctx) api returning the Group only?
  • Calling <-wg.Done() would be an equivalent to wg.Wait(),
  • No need for a particular Context() function (wg is the context),
  • Satisfying a context.Context value is a strong signal that the group isn't intended for reuse, any cleanup could be done in Wait/Done (e.g. no defer wg.Stop() needed).

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 func(ctx) error as the parameter for Go(). As far as I'm aware the x/ packages aren't subject to backwards compatibility promises, which might be an opportunity to provide a context-first API, instead of augmenting it with XContent() functions as I originally suggested. I think over 2000+ consumers of the errgroup package might be a deal-breaker here tho,... Nobody wants that amount of backlash on a breaking change 😄

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 8, 2020

*errgroup.Group should not satisfy context.Context. It has no need to carry values, and its Wait method may legitimately return nil (whereas a Context.Err method must always return a non-nil error after Done() is closed).

Loading

@rsc rsc added this to Incoming in Proposals Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants