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 Experimental HTTP/3 Support #1599

Merged
merged 30 commits into from Mar 16, 2023

Conversation

kckeiks
Copy link
Contributor

@kckeiks kckeiks commented Aug 6, 2022

#1558

  • Add small pool of HTTP/3 connections.
  • Using (Rustls) TLS config for setting up connection.
  • Use the SendRequest to start sending a request.
  • Reuse DNS resolvers.

It would be great to get some feedback. Thanks in advance.

@kckeiks
Copy link
Contributor Author

kckeiks commented Aug 6, 2022

Reuse DNS resolvers.

Resolution is done internally in hyper so there is no way to reuse the resolvers without re-implementing most of what is already in hyper. Thoughts?

Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

This looks amazing so far 🤩

CI wants a rustfmt run.

Cargo.toml Outdated Show resolved Hide resolved
src/async_impl/client.rs Outdated Show resolved Hide resolved
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Looking real good!

src/async_impl/client.rs Outdated Show resolved Hide resolved
src/async_impl/client.rs Outdated Show resolved Hide resolved
src/async_impl/h3_client/mod.rs Outdated Show resolved Hide resolved
src/async_impl/h3_client/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
@kckeiks kckeiks force-pushed the add-http3-support branch 3 times, most recently from f069009 to b2b3e80 Compare August 13, 2022 01:57
Signed-off-by: Miguel Guarniz <mi9uel9@gmail.com>
@seanmonstar
Copy link
Owner

This is so so exciting. I've noticed you kept pushing changes, so I wasn't sure when you felt it was "ready". Or maybe you felt it always was and just kept tweaking things to be a little better while waiting for me, in which case sorry!

@kckeiks
Copy link
Contributor Author

kckeiks commented Aug 20, 2022

This is so so exciting. I've noticed you kept pushing changes, so I wasn't sure when you felt it was "ready". Or maybe you felt it always was and just kept tweaking things to be a little better while waiting for me, in which case sorry!

No problem! I did run some tests and found things that needed to be improved but now I think it's ready for another round of review. 😊

@seanmonstar
Copy link
Owner

Ok, this is phenomenal! Now I'm left wondering what the best way to expose an "experimental" features is...

  • Ideally it'd just require RUSTFLAGS=--cfg reqwest_unstable_h3 and we write cfg(reqwest_unstable_h3) internally.
  • But we can't publish a version to crates.io that has a git dependency on h3.
    • Is there a way around this?
      • Can we abuse a build script to check for the flag and then edit the Cargo.toml? I know, sounds horrible. 🤷
        • Probably won' work, though...
  • So, do we just publish 0.0.0-breaking-galore.1 versions of h3 that reqwest can depend on?
  • Any other suggestions? 🙈

@tshepang
Copy link
Contributor

tshepang commented Aug 25, 2022

Ideally it'd just require RUSTFLAGS=--cfg reqwest_unstable_h3 and we write cfg(reqwest_unstable_h3) internally.

Instead of leaving it behind feature flag, I'd say do a beta version and call for testing, and if an issue is found after a final is release that requires a breaking API, do a breaking release. You can just mention that it's experimental in docs to be clear it's fresh code.

@tshepang
Copy link
Contributor

* But we can't publish a version to crates.io that has a git dependency on `h3`.
  
  * Is there a way around this?
    
    * Can we abuse a build script to check for the flag and then edit the `Cargo.toml`? I know, sounds horrible. shrug
      
      * Probably won' work, though...

* So, do we just publish `0.0.0-breaking-galore.1` versions of `h3` that `reqwest` can depend on?

The h3 people can be encouraged to publish an alpha version perhaps.

@kckeiks
Copy link
Contributor Author

kckeiks commented Aug 25, 2022

So, do we just publish 0.0.0-breaking-galore.1 versions of h3 that reqwest can depend on?

Yes, I think this would be better. Please let me know if I can help with that over in h3.

@jirutka
Copy link

jirutka commented Oct 10, 2022

Using (Rustls) TLS config for setting up connection.

Can you please implement it also for native-tls?

@seanmonstar
Copy link
Owner

Native-tls uses openssl on Linux, and they don't have the needed methods to start a QUIC connection.

@seanmonstar
Copy link
Owner

Alright, sorry for it taking so long. We've just now published h3 and h3-quinn at v0.0.1, specifically to unblock this PR. (We expect using it here will point out some annoyances quickly, and a quinn upgrade too, so it's not meant for a wider audience).

Would you want to upgrade this PR to use that? If not, I can do it likely next week.

@kckeiks
Copy link
Contributor Author

kckeiks commented Mar 10, 2023

Alright, sorry for it taking so long. We've just now published h3 and h3-quinn at v0.0.1, specifically to unblock this PR. (We expect using it here will point out some annoyances quickly, and a quinn upgrade too, so it's not meant for a wider audience).

Would you want to upgrade this PR to use that? If not, I can do it likely next week.

Sure! I can work on it this coming week.

@kckeiks
Copy link
Contributor Author

kckeiks commented Mar 16, 2023

I ran some tests. Seems fine. Ready for review.

Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

❤️ 🎉 🚀

@seanmonstar seanmonstar merged commit 57a8a01 into seanmonstar:master Mar 16, 2023
30 checks passed
@araujo-luis
Copy link

araujo-luis commented Apr 28, 2023

Would it make sense to implement an "HTTP2 fallback" logic in case the server does not support HTTP3?

@seanmonstar
Copy link
Owner

Yes, that'd be great! I've opened #1810 to track.

@araujo-luis
Copy link

Yes, that'd be great! I've opened #1810 to track.

Great! Will try to solve it!

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

5 participants