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

Add http2 feature to allow disabling HTTP/2 support #1648

Closed
wants to merge 2 commits into from

Conversation

jirutka
Copy link

@jirutka jirutka commented Oct 9, 2022

The HTTP/2 support requires the h2 crate which is quite a heavy dependency.

Resolves #1137

All tests are green: https://github.com/jirutka/reqwest/actions/runs/3222094053 (requering approval to run CI jobs in pull requests from new contributors is quite annoying default).

@jirutka jirutka force-pushed the http2 branch 2 times, most recently from 8928d4b to 3e4fc50 Compare October 10, 2022 19:28
@seanmonstar
Copy link
Owner

This would indeed be a good addition, but it's a breaking change, so we need to bundle it with the next breaking release (not yet scheduled, but will follow hyper 1.0).

@jirutka
Copy link
Author

jirutka commented Oct 10, 2022

When do you expect that to happen? Are we talking about weeks, months or years?

@seanmonstar
Copy link
Owner

hyper 1.0 is a couple months away. After that, updating reqwest will happen soon after.

@jirutka
Copy link
Author

jirutka commented Oct 12, 2022

What if I revert the logic – add no-http2 feature to disable http2 when enabled? We can switch it back before releasing 1.0.

@seanmonstar
Copy link
Owner

Adding a feature like that can cause breakage. If anyone in the dependency tree enabled the feature, it would disable HTTP/2 for any other crate that expected it to work.

The HTTP/2 support requires the h2 crate which is quite a heavy
dependency.

Resolves seanmonstar#1137
@jonas-schievink
Copy link

FYI this breaks cargo build --features blocking --no-default-features, looks like some #[cfg]s in blocking.rs are missing.

@seanmonstar seanmonstar mentioned this pull request Feb 15, 2023
@jonas-schievink
Copy link

FYI, we've found a bug in this PR, where reqwest will fail to communicate with a hybrid HTTP1+2 server with TLS enabled when reqwest is built with HTTP2 disabled. This seems to happen when doing ALPN negotiation, so presumably all of the HttpVersionPref::All branches should be #[cfg]d to do tls.request_alpns(&["http/1.1"]); instead of the current tls.request_alpns(&["h2", "http/1.1"]);?

@seanmonstar
Copy link
Owner

This will be part of 0.12, and was merged in #2162.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-breaking-change Blocked: breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: the way to disable http2 support
3 participants