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.

Sign up for GitHub

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

Jump to bottom

Add Experimental HTTP/3 Support #1599

Merged
merged 30 commits into from
Mar 16, 2023
Merged

Add Experimental HTTP/3 Support #1599

merged 30 commits into from
Mar 16, 2023

Conversation

kckeiks
Copy link
Contributor

@kckeiks kckeiks commented Aug 6, 2022 •
edited
Loading

#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 •
edited
Loading

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?

@kckeiks
Add and hook up simplistic HTTP/3 Client
9896862 
Signed-off-by: Miguel Guarniz <[email protected]>
@kckeiks
Pass Rustls config from Builder to H3 client constructor
bcf2ddd 
Signed-off-by: Miguel Guarniz <[email protected]>
@kckeiks
Add Pool code from Hyper
797edbf 
Signed-off-by: Miguel Guarniz <[email protected]>
@kckeiks
Add connection pool to HTTP/3 Client
5dd0e87 
Signed-off-by: Miguel Guarniz <[email protected]>
@kckeiks
Construct http::Body from chunks when creating response
107372c 
Signed-off-by: Miguel Guarniz <[email protected]>
@kckeiks
Add feature flags to example
808c0cc 
Signed-off-by: Miguel Guarniz <[email protected]>
@kckeiks
Clean up code
d62d538 
Signed-off-by: Miguel Guarniz <[email protected]>
@kckeiks
Add HTTP/3-client builder
57c6ab8 
Signed-off-by: Miguel Guarniz <[email protected]>
@kckeiks
Use feature flag when creating Request for hyper and h3 clients
4a5f526 
Signed-off-by: Miguel Guarniz <[email protected]>
seanmonstar
seanmonstar requested changes Aug 6, 2022
View reviewed changes
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
@kckeiks
Add cfg to HTTP version enum
1b68cfc 
Signed-off-by: Miguel Guarniz <[email protected]>
@seanmonstar seanmonstar self-requested a review August 6, 2022 17:28
@kckeiks
Send payload data in requests
8e3819e 
Signed-off-by: Miguel Guarniz <[email protected]>
seanmonstar
seanmonstar requested changes Aug 8, 2022
View reviewed changes
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
@joepio joepio mentioned this pull request Aug 11, 2022
Open
@kckeiks kckeiks force-pushed the add-http3-support branch 3 times, most recently from f069009 to b2b3e80 Compare August 13, 2022 01:57
@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 •
edited
Loading

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.

@passcod passcod mentioned this pull request Sep 7, 2022
Closed
@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.

@kckeiks kckeiks mentioned this pull request Jan 19, 2023
Open
@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.

seanmonstar
seanmonstar approved these changes Mar 16, 2023
View reviewed changes
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
@araujo-luis
Copy link

araujo-luis commented Apr 28, 2023 •
edited
Loading

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

@seanmonstar seanmonstar mentioned this pull request Apr 28, 2023
Closed
@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!

@makorne makorne mentioned this pull request Jul 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanmonstar seanmonstar seanmonstar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kckeiks @seanmonstar @tshepang @jirutka @araujo-luis