-
Notifications
You must be signed in to change notification settings - Fork 111
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
feat(server): add AutoConnection #11
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.
Excellent start, thanks! I appreciate how the tests are really easy to follow, that it indeed works for both versions.
Do we need to bump the CI config to make those jobs happy? Could do so in a separate PR that is easier to review/merge.
I have no idea what is wrong with CI. It seems like the source is dependencies. |
I'm increasing the MSRV in #13, I think the Miri failures need those tests to be disabled for Miri ( |
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.
Hi, @seanmonstar pointed me at this MR since I'd implemented something similar as part of hyperium/hyper#3084, I've added some comments from comparing how I did it in hyper directly compared to how you've done things here.
What's the status here? I see you added some new commits, is this ready for a re-review? |
Yes. |
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 is excellent, I love it! I had one cosmetic idea that I wrote up inline.
|
||
impl<E> Http2Builder<'_, E> { | ||
/// Http1 configuration. | ||
pub fn http1(&mut self) -> Http1Builder<'_, E> { |
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.
Oh, now I see how you've done this. That's clever! I like the way it looks down in the unit tests.
I was slightly confused why both sub-builders had serve_connection
methods, and now seeing this one here, I understand. I have a suggestion, and while I kinda like it, I'm open to hearing why it might be worse:
What if each method on the sub-builder returned, not &mut Self
, but &mut Builder<E>
? The builders in Finagle work the same way: a sub-builder is only usable once, and then you get back the full thing. It does make it easier to access options that affect both versions, if we were to add any.
So:
Builder::new(exec)
.http1().keep_alive(true)
.http1().writev(true)
.http2().enable_connect_protocol(true)
.max_send_buf(16324) // hypothetical, applies to both versions
.serve_connection(io, svc);
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'm open to it. I think we should ask users somehow on which one they prefer. Personally, I can work with both.
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.
There was an informal poll, and the current API was preferred, so let's merge with that. Thanks for all the work putting this together!
@programatik29 would you have availability to resolve the merge conflicts? Or should I try to do so in a new PR? |
@seanmonstar |
@programatik29 fixed. |
Closes hyperium/hyper#2852.
This utility automatically detects HTTP protocol and uses its implementation.