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: context: add WithCancelReason #46273

Closed
OneOfOne opened this issue May 19, 2021 · 15 comments
Closed

proposal: context: add WithCancelReason #46273

OneOfOne opened this issue May 19, 2021 · 15 comments

Comments

@OneOfOne
Copy link
Contributor

  • Would you consider yourself a novice, intermediate, or experienced Go programmer?
    I'd say experienced

  • What other languages do you have experience with?
    C++, Javascript, Python, PHP

  • Would this change make Go easier or harder to learn, and why?
    I don't think it'd change how easy or hard it is.

  • Has this idea, or one like it, been proposed before?
    I think so? but for the life of me I couldn't find it.

  • Who does this proposal help, and why?
    People who use context.WithCancel a lot and need to pass more error-context than just ErrCanceled.

  • What is the proposed change?
    It adds a new type and a function to the context package.

// A CancelReasonFunc tells an operation to abandon its work with the specified error
// and it cannot be nil.
// A CancelReasonFunc does not wait for the work to stop.
// A CancelReasonFunc may be called by multiple goroutines simultaneously.
// After the first call, subsequent calls to a CancelFunc do nothing.
// The first error set is the one returned from Err().
type CancelReasonFunc func(reason error)

// WithCancelReason returns a copy of parent with a new Done channel. The returned
// context's Done channel is closed when the returned cancel function is called
// or when the parent context's Done channel is closed, whichever happens first.
//
// Canceling this context releases resources associated with it, so code should
// call cancel as soon as the operations running in this Context complete.
func WithCancelReason(parent Context) (ctx Context, cancel CancelReasonFunc) {}
  • Is this change backward compatible?
    Yes.

  • Show example code before and after the change.

Before:

func doStuff(ctx context.Context, errCh chan error) {
	select {
	case <-ctx.Done():
	case err := <-err:
		switch err {
		case ErrServerExploded:
			// find another server and retry again.
		}
	}
}

After:

func doStuff(ctx context.Context) {
	<-ctx.Done()
	switch err := ctx.Err() {
		case ErrCanceled:
		// nothing todo
		case ErrServerExploded:
		// find another server and retry again.
	}
}
  • What is the cost of this proposal? (Every language change has a cost).
    It doesn't add any overhead, the only cost is one extra function (+ type) in the context package.

  • Can you describe a possible implementation?
    https://go-review.googlesource.com/c/go/+/319892

  • How would the language spec change? No change

  • Orthogonality: how does this change interact or overlap with existing features?
    It would lower the mental overhead of passing errors down to children

  • Is the goal of this change a performance improvement?
    Possible slightly less memory allocation for that one extra channel we'd use to pass error details? not really much.

  • Does this affect error handling? In a way.

  • Is this about generics? No.

I'm not stuck on WithCancelReason, maybe WithError would be better?

@gopherbot gopherbot added this to the Proposal milestone May 19, 2021
@ianlancetaylor ianlancetaylor changed the title proposal: add context.WithCancelReason proposal: context: add WithCancelReason May 20, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 20, 2021
@sfllaw
Copy link
Contributor

sfllaw commented Sep 29, 2021

What would be the canonical way of using this? I’m trying to understand the usage:

func foo(ctx context.Context) error {
	ctx, cancel := context.WithCancelReason(ctx)
	defer cancel(nil)

	// doStuff waits for dial:
	go doStuff(ctx)

	if err := dial(ctx); err != nil {
		cancel(fmt.Errorf("foo: %w", err))
		return err
	}

	return nil
}

The above doc-comment suggests that cancel(nil) is not valid, but almost every context.WithCancel I’ve seen runs defer cancel() to clean up. Shouldn’t context.WithCancelReason use the same mechanism so that the goroutine doesn’t leak?

I’m still struggling to understand why we’d implement doStuff to wait unconditionally for the context to finish. Perhaps you could provide a richer example?

@rsc
Copy link
Contributor

rsc commented Nov 10, 2021

It seems like this can be done as a wrapper around WithCancel, returning a Context wrapper with a custom Err function.

How often does this come up? Why is it important enough to add to the standard package?

@rsc
Copy link
Contributor

rsc commented Nov 10, 2021

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

@rsc rsc moved this from Incoming to Active in Proposals (old) Nov 10, 2021
@OneOfOne
Copy link
Contributor Author

OneOfOne commented Nov 10, 2021 via email

@ianlancetaylor
Copy link
Contributor

@OneOfOne What @rsc is saying is that you can do this yourself. You don't need a change in the standard library.

The question is: does this come up often enough that it should go into the standard library? This question is mainly for other people.

@ChrisHines
Copy link
Contributor

ChrisHines commented Nov 11, 2021

The documentation for Context.Err() currently says:

// If Done is not yet closed, Err returns nil.
// If Done is closed, Err returns a non-nil error explaining why:
// Canceled if the context was canceled
// or DeadlineExceeded if the context's deadline passed.
// After Err returns a non-nil error, successive calls to Err return the same error.
Err() error

If we add WithCancelReason how do you propose to update the docs for the Err method to reflect the new behavior?

@sfllaw
Copy link
Contributor

sfllaw commented Nov 11, 2021

The question is: does this come up often enough that it should go into the standard library? This question is mainly for other people.

I can hazard a guess, which is related to #26356. Some of my coworkers have complained that they can’t figure out why something got cancelled, so this is really a request that cancellations explain themselves, in the form of a replacement to WithCancel.

The reason people believe it should belong in the standard library is that they want the standard library to force people to write these explanations. That’s also why #26356 (comment) and others mention stack traces, which is another facet to the same problem.

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

It still sounds like doing this in an external implementation of Context would still be the place to experiment with a change like this.

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

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

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Dec 1, 2021
@egorbunov
Copy link

egorbunov commented Dec 3, 2021

It seems like this can be done as a wrapper around WithCancel, returning a Context wrapper with a custom Err function.

How often does this come up? Why is it important enough to add to the standard package?

Let me show you why this is not true and support from stdlib may help.

Imagine we have WithCancelReason function somehow implemented. My particular case can be described like this:

func doOperation(ctx context.Context) error {
    ctx, cancelReason := WithCancelReason(ctx)

    // there is a background process which periodically checks
    // that we still are allowed to perform an operation
    go func() {
        for {
            if checkAccessLost() {
                cancelReason(ErrAccessLost)
                return
            }
        }
    }()

    return thirdPartyCall(ctx)
}

func thirdPartyCall(ctx context.Context) error {
    ctx, close := context.WithTimeout(ctx, time.Minute)
    defer close()
    
    for {
        if ctx.Err() != nil {
            return ctx.Err()
        }
     }
}

Imagine now that cancelReason(ErrAccessLost) is executed before timeout inside thirdPartyCall occurs.
I would expect in such case that inside thirdPartyCall, ctx.Err() returns ErrAccessLost.

But standard context.WithTimeout returns context.Context which can only return Canceled or DeadlineExceeded from Err() method!

My conclusion here: it is impossible to implement WithCancelReason as an external addition to standard context package in a satisfactory way.

@sfllaw
Copy link
Contributor

sfllaw commented Dec 4, 2021

@egorbunov

My conclusion here: it is impossible to implement WithCancelReason as an external addition to standard context package in a satisfactory way.

Your example seems to work for me, after I hacked together a version of WithCancelReason: https://go.dev/play/p/PXWS2Z0AU_X

func doOperation(ctx context.Context) error {
	ctx, cancelReason := WithCancelReason(ctx)

	// there is a background process which periodically checks
	// that we still are allowed to perform an operation
	go func() {
		cancelReason(errors.New("access lost"))
	}()

	return thirdPartyCall(ctx)
}

func thirdPartyCall(ctx context.Context) error {
	ctx, close := context.WithTimeout(ctx, time.Second)
	defer close()

	for {
		if ctx.Err() != nil {
			return ctx.Err()
		}
	}
}

// Output:
// access lost

@egorbunov
Copy link

egorbunov commented Dec 4, 2021

Your example seems to work for me, after I hacked together a version of WithCancelReason: https://go.dev/play/p/PXWS2Z0AU_X

Thank you very much, @sfllaw, your are right. I was not careful enough looking at stdlib cancelCtx code and how it "inherits" parent.Err().

@egorbunov
Copy link

Your example seems to work for me, after I hacked together a version of WithCancelReason: https://go.dev/play/p/PXWS2Z0AU_X

Thank you very much, @sfllaw, your are right. I was not careful enough looking at stdlib cancelCtx code and how it "inherits" parent.Err().

Despite the fact it is doable in an external package I still want to emphasize:

  1. Such implementation is not just a simple wrapper as supposed by @rsc. In @sfllaw implementation it is a copy-paste of a cancelCtx related code from standard library with all the logic of cancel propagation.
  2. Implementing it externally leads to inability to reuse cancelCtxKey between standard cancelCtx and an external one. Thus, extra goroutines (in popagateCancel) will be spawned when wrapping external WithCancelReason context with stdlib WithCancel/WithTimeout.

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Dec 8, 2021
@rsc
Copy link
Contributor

rsc commented Dec 8, 2021

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

@rsc
Copy link
Contributor

rsc commented Mar 9, 2022

For people interested in this topic, please take a look at #51365 and see if it addresses your use cases. Thanks!

@golang golang locked and limited conversation to collaborators Mar 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

7 participants