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

hyper v1 upgrade #2059

Merged
merged 1 commit into from Mar 8, 2024
Merged

hyper v1 upgrade #2059

merged 1 commit into from Mar 8, 2024

Conversation

seanmonstar
Copy link
Owner

@seanmonstar seanmonstar commented Dec 7, 2023

The upgrade process to hyper v1, which is tracked in #2039. This branch isn't compiling yet, which is why it's marked as a draft. But I'm doing it in pieces, and if someone wants to grab some while I'm distracted with other things, it's here.

Closes #2039

@loispostula
Copy link
Contributor

@seanmonstar What still needs to be done, can we help?

With https://github.com/rustls/hyper-rustls/releases/tag/v%2F0.26.0 it should permit to progress

@jakubadamw
Copy link
Contributor

@seanmonstar I added a bunch of commits to this fork: https://github.com/grafbase/reqwest/commits/hyper-v1-and-wasm-from-parts-build-split/. All but the newest one ("WIP") might be useful. Some might need adjustment, but the fork works for us. 🙂 The test suite isn't passing yet, but a lot of tests were green after the adjustment in the "WIP" commit.

@seanmonstar
Copy link
Owner Author

Thanks! If you wanna open a PR against this branch with the working commits, I'll merge it in ♥️

@jakubadamw
Copy link
Contributor

@seanmonstar sure, of course! Here it is: #2115. They're a somewhat minimal set of changes that I had to make in order to get reqwest working for us with hyper v1, but they may be neither sufficient, nor necessarily entirely correct. They're also dependent on hyperium/hyper-util#95, which I opened separately. Kindly let me know if I can be of more help. 🙂

@seanmonstar seanmonstar force-pushed the hyper-v1 branch 4 times, most recently from 4354bde to 68a683d Compare January 31, 2024 21:13
@couchand
Copy link

@seanmonstar Thanks for working on this! I'd like to draw your attention to the fact that the current draft commit seems to have omitted @jakubadamw's e-mail from the Co-authored-by: line.

@seanmonstar
Copy link
Owner Author

Yes you're right, I was fiddling trying to get all the authors, and was confused by the unverified badge, but got interrupted before settling it. It's sitting in an uncommitted state locally. Thanks for the reminder!

@seanmonstar seanmonstar force-pushed the hyper-v1 branch 4 times, most recently from 4d0d28b to a819add Compare February 19, 2024 15:04
@seanmonstar seanmonstar marked this pull request as ready for review February 19, 2024 15:15
@seanmonstar
Copy link
Owner Author

Alright, everything else passes, I disabled the http3 feature. I could be convinced otherwise, but I'm currently leaning towards moving forward without http3, since it was "unstable" for exactly that reason. We can re-enable once it works again.

@seanmonstar seanmonstar changed the title wip: hyper v1 upgrade hyper v1 upgrade Feb 19, 2024
rustls_pemfile::Item::X509Certificate(cert) => {
certs.push(rustls::Certificate(cert))
rustls_pemfile::Item::X509Certificate(cert) => certs.push(cert.into()),
rustls_pemfile::Item::PKCS8Key(key) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


fn supported_verify_schemes(&self) -> Vec<SignatureScheme> {
vec![
SignatureScheme::RSA_PKCS1_SHA1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: sort alphabetically?

});

root_cert_store.add_trust_anchors(trust_anchors);
root_cert_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: probably better extend_with_slice: https://github.com/seanmonstar/reqwest/pull/2139/files#diff-0df5342f97493f9f55d6f43a14268f327ea9e791c7fdb8df6d88ec81f6baa721R469

Avoiding clone and thus additional moves I guess


let mut buf = [0; 8192];
let mut pos = 0;

loop {
let n = conn.read(&mut buf[pos..]).await?;
let n = tokio_conn.read(&mut buf[pos..]).await?;

This comment was marked as resolved.

@smndtrl

This comment was marked as off-topic.

@seanmonstar

This comment was marked as off-topic.

@jakubadamw
Copy link
Contributor

@seanmonstar thanks for your work on this. Out of curiosity, is there any blocker remaining to this getting merged? 🙂

@seanmonstar
Copy link
Owner Author

The last thing before merging I need to figure out: releasing what's merged since the last release (and do I bother with deprecating/renaming builder options that have inconsistent names), and potential support plan for the "old" version. But some other higher urgency things have my attention this week.

This is a big and breaking change. A lot of the internals were worked
on. But the breakage shouldn't be that visible to public API.

The main differences are:

- Publicly exposes `http` v1, instead of v0.2.
- Integration with `hyper::Body` has changed.

Co-authored-by: Sean McArthur <sean@seanmonstar.com>
Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <lois@postu.la>
Co-authored-by: Jakub Wieczorek <jakub@grafbase.com>
@seanmonstar
Copy link
Owner Author

Alright, v0.11.25 is out, maybe there were some other things that could have been squeezed in, but oh well. Let's get this merged! 🚀

@seanmonstar seanmonstar merged commit 2c11ef0 into master Mar 8, 2024
32 checks passed
@seanmonstar seanmonstar deleted the hyper-v1 branch March 8, 2024 22:07
@Venryx
Copy link

Venryx commented Mar 9, 2024

To clarify for others: It looks like v0.11.25 was released prior to the merging of the pull-request that updates hyper to v1.

Reason this came to my attention: I tried updating to the "hyper v1 compatible" version of reqwest by adding this to my Cargo.toml:

reqwest = "0.11.25"

I waited for rust-analyzer to finish processing, then inspected the Cargo.lock file, and noticed these contents:

[[package]]
name = "reqwest"
version = "0.11.25"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0eea5a9eb898d3783f17c6407670e3592fd174cb81a10e51d4c37f49450b9946"
dependencies = [
 [...]
 "hyper 0.14.28",                <-- the unexpected line
 "hyper-rustls 0.24.2",
 "hyper-tls",
 [...]
]

So I guess a version of reqwest that depends on hyper v1 has just not been released yet?

If so, I'll just continue using GitHub as the crate source, but figured I'd mention this here for others who might have thought that v0.11.25 was supposed to support hyper v1.


For others: This is how you can include the "hyper v1 compatible" version of reqwest in your project: (add this to your Cargo.toml)

reqwest = {git = "https://github.com/seanmonstar/reqwest.git", rev = "2c11ef000b151c2eebeed2c18a7b81042220c6b0", <set your features here> }

seanmonstar added a commit that referenced this pull request Mar 19, 2024
This is a big and breaking change. A lot of the internals were worked
on. But the breakage shouldn't be that visible to public API.

The main differences are:

- Publicly exposes `http` v1, instead of v0.2.
- Integration with `hyper::Body` has changed.

Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <lois@postu.la>
Co-authored-by: Jakub Wieczorek <jakub@grafbase.com>
seanmonstar added a commit that referenced this pull request Mar 19, 2024
This is a big and breaking change. A lot of the internals were worked
on. But the breakage shouldn't be that visible to public API.

The main differences are:

- Publicly exposes `http` v1, instead of v0.2.
- Integration with `hyper::Body` has changed.

Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <lois@postu.la>
Co-authored-by: Jakub Wieczorek <jakub@grafbase.com>
seanmonstar added a commit that referenced this pull request Mar 19, 2024
This is a big and breaking change. A lot of the internals were worked
on. But the breakage shouldn't be that visible to public API.

The main differences are:

- Publicly exposes `http` v1, instead of v0.2.
- Integration with `hyper::Body` has changed.

Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <lois@postu.la>
Co-authored-by: Jakub Wieczorek <jakub@grafbase.com>
seanmonstar added a commit that referenced this pull request Mar 20, 2024
This is a big and breaking change. A lot of the internals were worked
on. But the breakage shouldn't be that visible to public API.

The main differences are:

- Publicly exposes `http` v1, instead of v0.2.
- Integration with `hyper::Body` has changed.

Co-authored-by: =?UTF-8?q?Lo=C3=AFs=20Postula?= <lois@postu.la>
Co-authored-by: Jakub Wieczorek <jakub@grafbase.com>
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.

Upgrade to hyper 1.0
8 participants