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

feat(server): add AutoConnection #11

Merged
merged 14 commits into from
Sep 16, 2023

Conversation

programatik29
Copy link
Contributor

@programatik29 programatik29 commented Nov 5, 2022

Closes hyperium/hyper#2852.

This utility automatically detects HTTP protocol and uses its implementation.

@programatik29 programatik29 changed the title feat(server): add AutoConn feat(server): add AutoConnection Nov 18, 2022
Copy link
Member

@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.

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.

src/server/conn.rs Outdated Show resolved Hide resolved
src/server/conn.rs Outdated Show resolved Hide resolved
src/server/conn.rs Outdated Show resolved Hide resolved
src/server/conn.rs Outdated Show resolved Hide resolved
@programatik29
Copy link
Contributor Author

Do we need to bump the CI config to make those jobs happy?

I have no idea what is wrong with CI. It seems like the source is dependencies.

@seanmonstar
Copy link
Member

I'm increasing the MSRV in #13, I think the Miri failures need those tests to be disabled for Miri (#[cfg(not(miri))]).

Copy link

@bossmc bossmc left a 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.

src/server/conn/auto.rs Outdated Show resolved Hide resolved
src/server/conn/auto.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/server/conn/auto.rs Outdated Show resolved Hide resolved
@seanmonstar
Copy link
Member

What's the status here? I see you added some new commits, is this ready for a re-review?

@programatik29
Copy link
Contributor Author

What's the status here? I see you added some new commits, is this ready for a re-review?

Yes.

Copy link
Member

@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 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> {
Copy link
Member

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);

Copy link
Contributor Author

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.

Copy link
Member

@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.

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!

@seanmonstar
Copy link
Member

@programatik29 would you have availability to resolve the merge conflicts? Or should I try to do so in a new PR?

@programatik29
Copy link
Contributor Author

@seanmonstar client module is generating errors which isn't related to this PR.

@seanmonstar
Copy link
Member

@programatik29 fixed.

@seanmonstar seanmonstar merged commit 334209d into hyperium:master Sep 16, 2023
15 checks passed
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.

Make an Auto (or Either) server Connection type
4 participants