CC @bcmills

The errgroup context is provided as a convenience. If it's not convenient, isn't it possible to create the context separate from the errgroup? It does't seem like errgroup needs to change.

That is, use context.WithCancel and then call that cancel func instead of errgroup.Cancel in the rewrite.

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

I don't think this is ideal because Group.Go does not check if the ctx was canceled before calling f, thus all queries/execs in the transaction would be executed

That is true even with errgroup.WithContext today — so I don't see that this proposal would make that approach any better.

The errgroup context is provided as a convenience. If it's not convenient, isn't it possible to create the context separate from the errgroup? It does't seem like errgroup needs to change.

That is, use context.WithCancel and then call that cancel func instead of errgroup.Cancel in the rewrite.

You are right, it is possible indeed.

That is true even with errgroup.WithContext today — so I don't see that this proposal would make that approach any better.

That's true. I'd like to rephrase what I said in the original post and give another example to better illustrate my point.

My original post and the title was a bit misleading as to what I wanted to say, I hope to make it clearer this time. Sorry.

While it is true that one could use a different context, I don't think it is a better solution than my proposed one.
More so if the funcs called by the Go method can't be grouped inside a loop.

For example, currently, this is how this could be achieved, as proposed by @rsc, if I understood it correctly:

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

errGroup := errgroup.Group{}
tx, _ := db.BeginTx(ctx, &sql.TxOptions{})

errGroup.Go(func() error {
	var err error
	defer func() {
		if err != nil {
			cancel()
		}
	}()

	_, err = tx.ExecContext(ctx, "exec one")
	return err
})

errGroup.Go(func() error {
	var err error
	defer func() {
		if err != nil {
			cancel()
		}
	}()

	_, err = tx.ExecContext(ctx, "exec two")
	return err
})

errGroup.Go(func() error {
	var err error
	defer func() {
		if err != nil {
			cancel()
		}
	}()

	_, err = tx.ExecContext(ctx, "exec three")
	return err
})

if err := errGroup.Wait(); err != nil {
	tx.Rollback()
	return
}

tx.Commit()

The context would only be canceled in case of an error, because the errorgroup is not using it, and the commit would work just fine.
We are also avoiding unnecessary database calls, because the context is being passed to the exec calls.

But, imo, this looks too repetitive.
With my proposed change, it would look like this:

errGroup, ctx := errgroup.WithContext(context.Background())
defer errGroup.Cancel()
tx, _ := db.BeginTx(ctx, &sql.TxOptions{})

errGroup.Go(func() error {
	_, err := tx.ExecContext(ctx, "exec one")
	return err
})

errGroup.Go(func() error {
	_, err := tx.ExecContext(ctx, "exec two")
	return err
})

errGroup.Go(func() error {
	_, err := tx.ExecContext(ctx, "exec three")
	return err
})

if err := errGroup.WaitNoCancel(); err != nil {
	tx.Rollback()
	return
}

tx.Commit()

This is cleaner and more convinient, imo.

The method Go cancels the context if f errors out. So the user doesn't have to worry about cancelig the context. This avoid code repetition.

And it doesn't matter that the method Go doesn't check the ctx before calling f, because in this case the ctx is passed to the transaction and the exec calls.
In casy any f fails, all subsequent execs would give a ctx error instead of executing the call. And this would have been handled by the errgroup package, instead of manually and repetitively by the user.

Up to this point, this is all doable with the current implementation of the errgroup package.
The trick part here is the WaitNoCancel method. Without it, the commit would always fail, because calling Wait instead would guarantee that the context would always be canceled, even if no error occurred.

What is the difference between your proposed approach and this one?

txCtx, cancel := context.WithCancel(context.Background())
defer cancel()
tx, _ := db.BeginTx(txCtx, &sql.TxOptions{})

g, ctx := errgroup.WithContext(txCtx)

g.Go(func() error {
	_, err := tx.ExecContext(ctx, "exec one")
	return err
})

g.Go(func() error {
	_, err := tx.ExecContext(ctx, "exec two")
	return err
})

g.Go(func() error {
	_, err := tx.ExecContext(ctx, "exec three")
	return err
})

if err := g.Wait(); err != nil {
	tx.Rollback()
	return
}

tx.Commit()

What is the difference between your proposed approach and this one?

I didn't think of that approach. That's a good point.
Guess the only difference is dealing with a single context, instead of two. But I don't think it's enough to justify the change. Nor do I think it's a bad thing to be honest.

Thanks for the feedback!

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

No change in consensus, so declined.
— rsc for the proposal review group