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

Sync documentation for hyper_util::server::conn::auto::Http1Builder::header_read_timeout with hyper's hyper::server::conn::http1::Builder::header_read_timeout. #137

Merged

Conversation

aaronriekenberg
Copy link
Contributor

@aaronriekenberg aaronriekenberg commented Jul 12, 2024

I think the default of 30 seconds, and the commentary about needing to set Timer to avoid panics are true for hyper_util::server::conn::auto::Builder? I copied documentation from Hyper here: https://github.com/hyperium/hyper/blob/master/src/server/conn/http1.rs#L311-L323

A possibly larger question - should auto::Builder do something to setup a Timer by default for http1/http2 builders? Already auto::Builder::new takes an executor parameter, should this also take a timer parameter?

Thanks! 😃

@aaronriekenberg aaronriekenberg changed the title Sync documentation for Http1Builder::header_read_timeout with hyper's Builder::header_read_timeout. Sync documentation for hyper_util::server::conn::auto::Http1Builder::header_read_timeout with hyper's hyper::server::conn::http1::Builder::header_read_timeout. Jul 13, 2024
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.

Thanks for improving the docs! I really appreciate it <3

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

A possibly larger question - should auto::Builder do something to setup a Timer by default for http1/http2 builders? Already auto::Builder::new takes an executor parameter, should this also take a timer parameter?

Hm, I suppose it could be explored and if it feels better, introduced as a breaking change in hyper-util 0.2 or something. One additional option is making use of typestate (such as what rustls does, such that if you want to set timeout options, you have to have set a timer, but it's therefore not required if you don't need timeouts.

impl<E> Builder<E, NeedsTimer> {
    pub fn timer<T>(self, timer: T) -> Builder<E, HasTimer> {
        // ...
    }
}

impl<E> Builder<E, HasTimer> {
    pub fn http1_read_timeout(self, timeout: Duration) -> Self {
        // ...
    }
}

No promises that we'd do that, but if that doesn't phase you, you're welcome to explore separately and see how it goes :)

@aaronriekenberg
Copy link
Contributor Author

Addressed review comments.

Will take a look at using typestate separately.

Thank you!

@seanmonstar seanmonstar merged commit b36538a into hyperium:master Jul 22, 2024
16 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.

2 participants