-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
context: add APIs for writing and reading cancelation cause #51365
Comments
// Cause returns a non-nil error if a parent Context was canceled using a CancelCauseFunc that was passed that error.
// Otherwise Cause returns nil.
func Cause(c Context) error It would be nice to use |
@wizardishungry That's right: the |
This proposal has been added to the active column of the proposals project |
I've needed this in multiple projects. |
I think, it could work if will be adopted by packages like net/http, sql, and so on. |
Hi Dmitry, what would integration with |
@Sajmani from the documentation https://pkg.go.dev/context#Context.Err it's not clear to me that it only returns those values. To me it looks like it just lists some of the usual errors you might get. I've always assumed that custom context implementations can return different errors. |
@Sajmani If that libraries would add a cancellation reason. |
@egonelbre While we might reasonably interpret the documentation of |
@Sajmani sure, I do fear that changing the behavior in that case would cause more problems. However, it also means that code checking I haven't found an easy way to find how common that problem is. One that I did find is https://github.com/pion/ice/blob/39ae3a4fa890e29069c764039a98dff44ca94255/context.go#L23 -- which suggests that there are more such contexts. |
FWIW, I have always interpreted the
I do prefer to use But that's not the end of it. In order for #46273 to be acceptable I think the My rationale is that the current docs pretty clearly (at least to me) specify that there are only two ways a context can become canceled and the The proposal sidesteps that by allowing contexts to provide two errors. Questions about this proposal: The proposed design of This seems maybe useful:
Now our logs will tell us which of the two timeouts triggered the work to abort. It may still be ambiguous in the case that the inner timeout ended up having the same deadline as its parent due to it potentially being started near the end of the parent context's time range. But what if
Now we lack the ability to provide a cause without adding another wrapping context like this:
This seems a bit unfortunate and I wonder if there is a better way. |
Great point about having causes both on deadline and direct cancelation. IMO the code that calls WithCancelCause and WithTimeoutCause is the clearest: it makes it clear that there are two different reasons why this context might be canceled and provides them both.
While you don't technically need to call the second cancel—since the first deferred cancel will cascade down to the derived context and cancel it—it seems like good hygiene to write it as you suggest. |
That example does suggest a synchronization consideration, though: both That is: there should be at most one reason attributed to the |
Does anyone object to adding this API? Specifically (quoting the top comment):
|
Which cancel cause will be reported if multiple parent contexts have been cancelled with cause? I’m assuming it’s the nearest parent? |
I would make the following changes and/or clarifications to the API:
(*) About this point, @Sajmani you've said, in response to @ChrisHines:
I personally don't buy it. A single call to |
Personally, I would prefer that it be left unspecified (because synchronization makes guarantees difficult), but implementations should try to preserve the |
@hherman1 @bcmills Indeed the behavior of the current prototype is that the cause associated with the first cancelation is propagated all the way down the chain, just like the Err is. This means a later cancelation, even if it's "nearer", won't update the cause, since the Context has already been canceled. |
@andreimatei Of your suggestions, #5 is the one I'm most in agreement with, as that's a fundamental issue with the ability to define custom Contexts. The rest are all attempts to make Cause(ctx) an alternative to ctx.Err(), which is not my proposal. IMO it's best for code to check ctx.Err() as they do today, and consult Cause(ctx) for additional information. I'm happy to hear other points of view on this, of course, and if the proposal committee prefers your semantics, I'm OK with that. |
On the one hand I'd like to enable custom contexts, but on the other hand I am not sure about exposing the implementation detail that is the context key. What stops someone from using WithCancelCause and then wrapping the result with their own implementation? Why do we need to expose the key? |
Nothing technically stops a custom ctx implementation from wrapping
I'd also bring a principle argument: |
Right, and that's a larger API change, which it would be good to avoid, at least at first. What if we want to optimize the implementation at some future point? How often do custom contexts need this and would it be so awful for them to call WithCancelCause? |
No change in consensus, so accepted. 🎉 |
Change https://go.dev/cl/426215 mentions this issue: |
@Sajmani Hello. In #51365 (comment) rsc suggested about Cause will return Context.Err if no cancel cause, but context cancelled. In finish proposed Api changes and in PR - Cause will return nil if no Cancel cause (even if context cancelled) |
@rekby Cause should return nil if no cause is set |
Change https://go.dev/cl/375977 mentions this issue: |
@rekby Actually I had forgotten Russ's comment; you are correct that Cause will return the same value as Err unless cause is set explicitly to a non-nil error. The current CL implements this behavior. |
I think @ChrisHines is correct that we cannot easily arrange for a timeout or deadline to cancel a context with a custom cause, at least, not without an extra goroutine or timer. @ianlancetaylor 's sketch in #51365 (comment) doesn't handle the common case when a context is canceled before the timer expires. If we want to make this case work well, then I think we will need |
I have submitted the CL to add WithCancelCause per @rsc 's spec in #51365 (comment) I'm reopening this issue so that the proposal committee can discuss whether we should add |
@Sajmani wouldn't that be a new proposal? Since (business/community related) value is already achieved with the currently proposal as implemented right now and the proposed additions are optimizations on top that can get done in the next cycle. If you propose to not ship the current version without the two additions discussed, keeping it open starts making more sense to me. Otherwise let's be fair and open a new proposal. |
Closing this proposal as implemented but we can add a new one to focus specifically on WithDeadlineCause and WithTimeoutCause, which are subtly different from WithCancelCause. WithCancelCause lets the cancel func set the cause at the cancellation moment. WithDeadlineCause and WithTimeoutCause require you to say ahead of time what the cause will be when the timer goes off, and then that cause is used in place of the generic DeadlineExceeded. The cancel functions they return are plain CancelFuncs (with no user-specified cause), not CancelCauseFuncs, the reasoning being that the cancel on one of these is typically just for cleanup and/or to signal teardown that doesn't look at the cause anyway. That distinction makes sense, but it makes WithDeadlineCause and WithTimeoutCause different in an important, subtle way from WithCancelCause. We missed that in the discussion, and although @ChrisHines did point it out after my July 27 message, I still don't think I understood it well enough until a discussion with @Sajmani just now. Sameer is going to open a separate proposal for these two additions. |
I have been following this 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 What I mean is that I have code which 1) obtains the error from the context, then it b) 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:
I understand that originally
For I am not sure if we can still change |
If you have a reference to the context that was possibly canceled (and possibly you have it because you presumably passed it down the function call chain), you could do at (3) the same that you'd do at (1) and consider both the error and the context. Based on what I've seen of the bug tracker etiquette, I think a better place to continue the conversation is not this closed issue but the mailing list or forum/discord/slack/irc, and if there's a new proposal/feature request/bug we'd open a new GitHub issue. |
Not necessary, because the context at 3) might not be the cancelable context used in 1) (but its parent context).
I am trying to first see if I missed something obvious and gauge interest before I open a new proposal. |
I made #63759. |
This proposal addresses issue #26356 by extending
package context
with new functions for writing and reading the "cause" of the context's cancelation. This approach is similar to what was proposed in #46273.Design notes and alternatives
Naming
I chose the name
Cause
because it's short. Alternatives includeReason
(as in #46273) andWhyDone
.Cause is an error
Based on user feedback in #26356, we've made cause an
error
so that the programmer can include structured data and introspect it witherrors.Is/As
. Alternatives would be a string or a stack trace (captured automatically).Compatibility
As
package context
is covered by the Go 1 compatibility guarantee, we are unable to change any of the existing types or functions in the package. In particularCause
as a method of theContext
interfaceCancelFunc
type to take a causeWithCancel
,WithDeadline
, orWithTimeout
function signaturesErr
to return any values other thannil
,Canceled
, orDeadlineExceeded
Performance considerations
Based on user feedback, our goal is to introduce no new performance overhead for existing uses of
Context
(hence the rejection of the design that captured stack traces automatically for existing cancelations). We consider it acceptable to increase the size of some context implementation structs by a small amount (specifically, adding anerror
field tocancelCtx
).Cause(ctx) vs. ctx.Err()
@andreimatei pointed out that in the current prototype, it's possible for
Cause(ctx)
to be nil whenctx.Err()
is non-nil. This is working as intended: we expect users to checkctx.Err()
before readingCause(ctx)
. However, @andreimatei suggested users might want to checkCause(ctx)
instead ofctx.Err()
, in which case we'd need to guarantee thatCause(ctx)
is non-nil exactly whenctx.Err()
is non-nil. If we do this, I'd recommendCause(ctx) == ctx.Err()
at all times except when the cause has been explicitly set to non-nil. This would complicate the handling of code that sets the cause tonil
.Concerns for custom Context implementations
@andreimatei raised concerns about the fact that this design does not provide a way for custom
Context
implementations to control the behavior ofCause(ctx)
. As a result, it's possible forCause(ctx)
to return a non-nil error while the custom Context'sDone
channel has not yet been closed. I pointed out that a customContext
can controlCause(ctx)
by wrapping a standardContext
returned byWithCancelCause
; @andreimatei pointed out that this makes it difficult for customContext
implementations to achieve their efficiency goals.We could address this concern by exposing a key for
Context.Value
that looks up theCause
, which would allow customContext
implementations to override the lookup for that key. This would require changing the current prototype that reuses the existingcancelCtxKey
to find the cause.Proposed API changes
I've prototyped this change in CL https://go-review.googlesource.com/c/go/+/375977, including several tests to exercise various interleaving of cancelations with and without causes.
The text was updated successfully, but these errors were encountered: