-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
*: wrap ctx.Err() with a stack #29318
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, but it looks like the search and replace you did needs some more finesse.
Thanks for the review, lol, I'll go audit these more carefully now. |
f7bd5b2
to
b2a6645
Compare
RFAL |
b2a6645
to
aff4695
Compare
Generating stack traces is fairly expensive. I'm not sure if that will be a problem here, just pointing it out. I'm a bit reluctant to have this always on. Perhaps the wrapping should be done in a way that it is controlled by an environment variable. Just brainstorming here. I haven't thought through this enough. I'd guess that you'd rather get a stack trace from the goroutine that cancelled the context. I think that might be possible if we vendor |
Would using the For example, from @andreimatei:
|
I haven't measured the perf difference between |
It's all to common to get an error like "context canceled" without any helpful information about which context was canceled and when. This commit strives to improve the situation by at least including the stack trace of when the ctx.Err() was retrieved. Release note: None
aff4695
to
273dc9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, the more important stack is the one of the routine doing the canceling than the canceled one. For the cancelee, I think a message identifying its location ("ctx canceled while waiting for foo") is sufficient, instead of the stack.
Btw, I hope you've searched the code for places that handle context.ErrCanceled
explicitly. I know for example @vivekmenezes is just introducing one currently.
Reviewable status: complete! 0 of 0 LGTMs obtained
This isn't as good as #29429. |
29429: contextutil: add CancelWithReason r=jordanlewis a=jordanlewis contextutil.WithCancel now returns a new context type that holds its reason for cancellation when canceled with CancelWithReason instead of its normal cancelfunc. Alternative / new approach to #29318. Release note: None Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
It's all to common to get an error like "context canceled" without any
helpful information about which context was canceled and when. This
commit strives to improve the situation by at least including the stack
trace of when the ctx.Err() was retrieved.
Not sure who to include on this. It's a straw-man PR, also.
Release note: None