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

Fix panic in tokio client when encountering unsupported StatusCode #447

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

FSMaxB
Copy link
Contributor

@FSMaxB FSMaxB commented Oct 4, 2023

We had a panic in production because of a well known issue in http_types where StatusCode conversion can panic:

A proper fix probably involves switching away from http_types entirely, but I interrupted my initial attempt because that requires changes all over the code base (replacing Request, Method, Url and Body types).

Therefore I'm submitting this quick fix which just avoids the code path with the unwrap.

Checklist

http_types::StatusCode doesn't support all status codes that http::StatusCode supports, but still implements From<http::StatusCode>, just with an `unwrap`. This is unfortunate because it can cause the client to panic upon responses that contain status codes not explicitly supported by http_types.
@FSMaxB
Copy link
Contributor Author

FSMaxB commented Oct 4, 2023

I really don't think there's anything I can/should do about the verify-codegen test failure ...

@arlyon
Copy link
Owner

arlyon commented Oct 5, 2023

I am not sure why the codegen would be causing an issue. I will merge this and solve that separately, TY.

Do you know what status code was failing for you?

@arlyon arlyon merged commit e180a61 into arlyon:master Oct 5, 2023
15 of 16 checks passed
@FSMaxB
Copy link
Contributor Author

FSMaxB commented Oct 5, 2023

Sadly no, because the panic message didn't contain that information. But there's some mention of cloudflare status codes here: http-rs/http-types#507

@FSMaxB FSMaxB deleted the fix-status-panic branch October 5, 2023 10:33
@arlyon
Copy link
Owner

arlyon commented Oct 5, 2023

Seems like silly behaviour on both sides. I am ok using this as a workaround. Release 0.25.2 is publishing now :)

Edit: live!

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.

2 participants