proposal: context: context.Cause should return a wrapped error

[mitar]

(I previously made a comment about this which I am now turning into full proposal.)

I have been following the #51365 issue for some time and was really happy that it landed. But now I tried to finally update code to use the new API and I was a bit surprised by it, primarily that it makes it hard to work with logic which expects that the error is context.Canceled and at the same time be able to retrieve the error at the end.

What I mean is that I have code which 1) obtains the error from the context, then it 2) passes it through by returning the error through a chain of function call returns, and then 3) inspects the error and determines that it is about the context cancellation and wants to obtain the cause for it (e.g., log it).

Now, the tricky thing is that at point 3) I cannot know both if the error is context cancellation and what was the cause. I can do that only at point 1).

Options to me seems to be:

• At point 1) do errors.Join(ctx.Err(), context.Cause(ctx)), but this is ugly if there was no cause, because context.Cause(ctx) in that case returns the same as ctx.Err()`, so I am joining two same errors. So more logic to combine only if they are different. Also returned error message is ugly.
• I could use gitlab.com/tozd/go/errors's WrapWith which was made so that one error can be made a cause of another error, without modifying either of them. The error message is nicer, but the logic to check if they are different has still to be added.

I understand that originally ctx.Err could not return an context.Canceled error which would unwrap to the cause error to assure that err == context.Canceled continues to work. But why not make context.Cause return such an error already? So in my view, it would be better if context.Cause returned:

• Same error as ctx.Err if there is no cause.
• An error which satisfies both context.Is(context.Canceled) and can be unwrapped to the cause error. This can be done by having a custom Is method on the error.

For context.Cause returned errors we do not have to assure that err == context.Canceled holds.

I am not sure if we can still change context.Cause now? Probably not? But could we then introduce yet another context.CauseError or something, which would return an error with the behavior above? It can probably be made in 3rd party library as well, it is not too long, but I think it would be great to have it in standard library.

[davidmdm]

@mitar I have done something very similar in my own libraries, but instead of adding an Is method to my error type, the error that I use as a cause implements Unwrap and returns context.Canceled. See: https://github.com/davidmdm/xcontext/blob/5cfe26a1770c732b0312108a02bf66ebdae05212/signal.go#L48-L58

Perhaps context.Cause could return a CauseError that wraps context.Canceled and the error that was passed to CancelCause func. This way the error returned from context.Cause would hold true for each of these conditions: errors.Is(err, context.Canceled) and errors.Is(err, CustomCauseErr{}).

[mitar]

I have done something very similar in my own libraries, but instead of adding an Is method to my error type

Yes, when you control errors that works. But for something in stdlibrary you want effectively the behavior of errors.Join(causeErr, context.Canceled, so that errors.Is (and errors.As) works with both, but with better error message. So I think it is worthwhile to add an private context.canceledWithCause error which makes errors.Is and errors.As work with both. context.Cause (or new context.CauseError, if we are too late to change context.Cause) function would return then such an error.

a CauseError that wraps context.Canceled and the error that was passed to CancelCause func

Yes, that could also be an option, that instead of implementing custom Is and As handling on context.canceledWithCause it would simply wrap two errors with maybe a custom error message. I just feel that this might be nicer to semantically think "this is a context.Canceled which wraps the causeErr" than "this new error wraps both causeErr and context.Canceled).

Either way I am fine. This is a trivial helper function which primarily handles the case that blindly calling errors.Join on ctx.Err() and context.Cause(ctx) might give you a join of two same errors. Very ugly.

[davidmdm]

I was thinking along the lines of if cancel is called with nil it continues to do as it does today and return context.Canceled directly, and if it is called with a cause error, context.Cause could return an error similar to this:

type CauseError struct {
	cause error
}

func (err CauseError) Error() string {
	return err.cause.Error()
}

func (err CauseError) Unwrap() []error {
	return []error{context.Canceled, err.cause}
}

My point being is that creating a custom error to implement the Is/As interfaces opens up a lot more complexity than simply unwrapping both errors. This way Is and As work as intended and it is clearer. We don't even need to use errors.Join and deal with its text formatting.

[mitar]

I think we generally have the same idea in mind and are discussing now a very particular implementation detail.

Personally, for wrapping two errors, I would prefer (difference in Error()):

type CauseError struct {
	cause error
}

func (err *CauseError) Error() string {
	return fmt.Sprintf("%s: %s", context.Canceled.Error(), err.cause.Error())
}

func (err *CauseError) Unwrap() []error {
	return []error{context.Canceled, err.cause}
}

But I do think the following is cleaner:

type CauseError struct {
	cause error
}

func (err *CauseError) Error() string {
	return fmt.Sprintf("%s: %s", context.Canceled.Error(), err.cause.Error())
}

func (err *CauseError) Is(target error) bool {
	return target == context.Canceled || errors.Is(err.cause, target)
}

func (err *CauseError) As(target interface{}) bool {
	return errors.As(context.Canceled, target) || errors.As(err.cause, target)
}

func (err *CauseError) Unwrap() error {
	return err.cause
}

[mateusz834]

type CauseError struct {
	cause error
}

Is there any reason not to export the cause field?

[davidmdm]

I do like the Error() method returning a joint message of context canceled and the cause which is exactly what I have done in my own library. As for implementing Is/As/Unwrap or on only Unwrap, it's technically all the same. I would ask what's the point of your Unwrap method if you've custom implemented Is and As but these are nitpicks and probably not important at this point.

Overall I support this proposal.

Is there any reason not to export the cause field

There is no particular reason to not export it. Regardless to get to the field you will need to As an error into a CauseError and you may as well As it directly into the error you are expecting. But I think it would be fine to make it public.

[davidmdm]

I think this proposal is technically backwards incompatible because people could have written code like so:

ctx, cancel := context.WithCancelCause(context.Background())
cancel(SentinalError)

if context.Cause(ctx) == SentinalError {
  // handle sentinal error cause
}

That being said, it is now well-known that people should be using errors.Is/As and it would be nice to fix/improve this API with a little dash of hindsight, but is this a deal breaker?

[mitar]

That being said, it is now well-known that people should be using errors.Is/As and it would be nice to fix/improve this API with a little dash of hindsight, but is this a deal breaker?

That why in my proposal I also have option that we implement yet another function which I tentatively call errors.CauseError which can return CauseError. While errors.Cause stays for backwards compatibility.

Is there any reason not to export the cause field

I think it is better to provide cause through Unwrap than directly expose the field. It allows us to change internals in the future while using standard Unwrap method here to access it.

[neild]

We can't change how context.Cause works now. It's clearly documented as returning the error passed to the CancelCauseFunc.

That makes the question whether this is enough of a problem that it's worth introducing yet another way to get an error out of a Context. I think that's going to be tough to justify; having both ctx.Err and context.Cause(ctx) is confusing enough already; adding context.CauseError(ctx) is going to increase that confusion.