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

HTTP/3 panic if udp is not available #1942

Closed
NobodyXu opened this issue Aug 14, 2023 · 4 comments · Fixed by #1945
Closed

HTTP/3 panic if udp is not available #1942

NobodyXu opened this issue Aug 14, 2023 · 4 comments · Fixed by #1945
Labels
C-bug Category: bug. Something is wrong. This is bad! E-pr-welcome The feature is welcome to be added, instruction should be found in the issue.

Comments

@NobodyXu
Copy link
Contributor

NobodyXu commented Aug 14, 2023

From cargo-bins/cargo-binstall#1292 :

192.1 thread 'main' panicked at 'unable to create QUIC endpoint: Os *** code: 92, kind: Uncategorized, message: "Protocol not available" ***', /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/reqwest-0.11.18/src/async_impl/h3_client/connect.rs:41:58

@NobodyXu NobodyXu changed the title HTTP/3: Fallback to HTTP/2 if failed to establish connection. HTTP/3 panic if failed to establish connection Aug 14, 2023
@NobodyXu
Copy link
Contributor Author

The user run this in docker and errno 92 means protocol not available, meaning udp not available.

@NobodyXu NobodyXu changed the title HTTP/3 panic if failed to establish connection HTTP/3 panic if udp is not available Aug 14, 2023
@NobodyXu
Copy link
Contributor Author

Given that it's not unheard of for udp to be banned, and now it seems to be banned within docker, I think it's a good idea to make it "Option<>"nal.

@seanmonstar
Copy link
Owner

We should change it to return an error at least, instead of panic. An eventual feature improvement could allow falling back to other protocols, but only if someone configures they want to allow that; http3_prior_knowledge implies "it MUST use http3". Panics are still bad, though!

@seanmonstar seanmonstar added C-bug Category: bug. Something is wrong. This is bad! E-pr-welcome The feature is welcome to be added, instruction should be found in the issue. labels Aug 15, 2023
@NobodyXu
Copy link
Contributor Author

An eventual feature improvement could allow falling back to other protocols, but only if someone configures they want to allow that

IMO fallback to other protocol should be the default, our user of cargo-binstall discovers this when running cargo-binstall inside docker and is probably blocked by the seccomp filer docker installed.

Also many corp env do the same and block udp, so fallback should be the default unless turned off by http3_prior_knowledge.

NobodyXu added a commit to NobodyXu/reqwest that referenced this issue Aug 19, 2023
Fixed seanmonstar#1942

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
seanmonstar pushed a commit that referenced this issue Aug 21, 2023
Fixed #1942

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug. Something is wrong. This is bad! E-pr-welcome The feature is welcome to be added, instruction should be found in the issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants