-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: add H3 client config support #2609
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great start! I appreciate the work, and it's quite thorough. I left some thoughts inline.
/// and accommodate future changes without breaking existing implementations. | ||
#[cfg(feature = "http3")] | ||
#[cfg_attr(docsrs, doc(cfg(all(reqwest_unstable, feature = "http3",))))] | ||
pub fn http3_send_grease(mut self, enabled: bool) -> ClientBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTP/2 doesn't really have much of a concept of grease. Like, maybe someone somewhere wishes it did, but it wasn't embraced nearly as well as it was for HTTP/3. For this method, I'd make the first sentence brief, like "Enable whether to send HTTP/3 protocol grease on the connections." And then in a second paragraph, explain what it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for your feedback and guidance. I will optimize the doc and resubmit it.
I encountered issues with some cloud provider cdn where enabling QUIC caused request failures due to the absence of grease.
src/async_impl/client.rs
Outdated
/// for extended CONNECT in HTTP/3; instead, the SETTINGS_ENABLE_WEBTRANSPORT setting implies that an endpoint supports extended CONNECT. | ||
#[cfg(feature = "http3")] | ||
#[cfg_attr(docsrs, doc(cfg(all(reqwest_unstable, feature = "http3",))))] | ||
pub fn http3_enable_extended_connect(mut self, enabled: bool) -> ClientBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option, and the next one (datagrams), should probably only be added once the user can make use of them in a request. So for now, I'd leave them out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already removed it.
Co-authored-by: Sean McArthur <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
cc hyperium/h3#206 .