-
Notifications
You must be signed in to change notification settings - Fork 932
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
set canceled status code if the underlying hyper error was due to cancelation #1669
Conversation
@LucioFranco gentle ping in case any notifications about these got buried. Merging this and a new patch release would be super helpful |
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.
Makes sense, thanks for the ping!
I can try to get a patch release out tomorrow, but I need to check the main branch otherwise I am out till mid next week when I can spend more time on this. |
@nrxus seems like there is a CI issue |
@LucioFranco Hm weird, it doesn't seem to be related to this PR at all. It looks like the latest rust is just a little stricter around dead code for tuple structs. I added an |
@LucioFranco another ping since I think it's all ready (: |
e4064f8
to
17a8d34
Compare
@LucioFranco I've rebased to latest master that fixed the clippy issue. I believe it's ready to merge but it needs you to hit approve again. |
@djc do you mind reviewing this? |
This makes sense to me, sorry for the long delays! |
Motivation
At work we use
tonic
to query CRI sockets about k8s information when available. This information is best effort only so we try to ignore "expected" errors while still logging errors we wouldn't expect to catch bugs. One of these expected errors is cancellation (e.g., due to a timeout) so we have code to check the status code. However, it appears that if the cancellation came about from a hyper envelope being dropped then the error is emitted asUnknown
fromtonic
. Pretty printed the error looks like:It seems right now there is special code for hyper errors whose source is an
h2::Error
, but the errors in the envelope drop have as a source just a static string so it defaults back to the "unknown" error.Solution
Thankfully
hyper:Error
has ais_canceled
method that checks its innerkind
.My apologies for the lack of test but it appears that
hyper:Error
isn't instantiatable from outside thehyper
crate so there wasn't an easy way to reproduce my specific kind of error in an unit test.