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

set canceled status code if the underlying hyper error was due to cancelation #1669

Merged
merged 2 commits into from
May 28, 2024

Conversation

nrxus
Copy link
Contributor

@nrxus nrxus commented Mar 29, 2024

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 as Unknown from tonic. Pretty printed the error looks like:

status: Unknown, message: "transport error", details: [], metadata: MetadataMap { headers: {} }: transport error: operation was canceled: connection closed: connection closed

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 a is_canceled method that checks its inner kind.

My apologies for the lack of test but it appears that hyper:Error isn't instantiatable from outside the hyper crate so there wasn't an easy way to reproduce my specific kind of error in an unit test.

@nrxus
Copy link
Contributor Author

nrxus commented Apr 4, 2024

@LucioFranco gentle ping in case any notifications about these got buried. Merging this and a new patch release would be super helpful

Copy link
Member

@LucioFranco LucioFranco left a 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!

@LucioFranco
Copy link
Member

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.

@LucioFranco
Copy link
Member

@nrxus seems like there is a CI issue

@nrxus
Copy link
Contributor Author

nrxus commented Apr 4, 2024

@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 #[allow(dead_code)] for the test struct that was causing issues.

@nrxus
Copy link
Contributor Author

nrxus commented Apr 12, 2024

@LucioFranco another ping since I think it's all ready (:

@nrxus nrxus force-pushed the fix-hyper-errors-cancelled branch from e4064f8 to 17a8d34 Compare April 26, 2024 20:59
@nrxus
Copy link
Contributor Author

nrxus commented Apr 26, 2024

@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.

@nrxus nrxus requested a review from LucioFranco April 26, 2024 21:01
@nrxus
Copy link
Contributor Author

nrxus commented May 28, 2024

@djc do you mind reviewing this?

@djc djc added this pull request to the merge queue May 28, 2024
@djc
Copy link
Collaborator

djc commented May 28, 2024

This makes sense to me, sorry for the long delays!

Merged via the queue into hyperium:master with commit b35d20d May 28, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants