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

fix(http2): gate server Builder keep-alive interfaces #3816

Merged

Conversation

cratelyn
Copy link

currently, dependents of hyper 0.14 that enable the server and http2 feature flags will see an error if they enable the backports flag.

building hyper like so...

cargo check --features server,http2,backports

...will yield the following errors (shown in "short" format here for brevity):

src/server/conn.rs:75:9: warning: unused import: `tracing::trace`
src/server/conn/http2.rs:185:25: error[E0609]: no field `keep_alive_interval` on type `h2::server::Config`
src/server/conn/http2.rs:199:25: error[E0609]: no field `keep_alive_timeout` on type `h2::server::Config`
warning: `hyper` (lib) generated 1 warning
error: could not compile `hyper` (lib) due to 2 previous errors; 1 warning emitted

this stems from the fact that the deprecated connection builder hyper::server::conn::Builder and the backported
hyper::server::conn::http2::Builder both operate on the same internal Config structure. this structure has changed slightly between 0.14 and 1.0 however. see:

; git diff 0.14.x master -- src/proto/h2/server.rs | grep 'struct Config' -A 11
@@ -51,12 +51,11 @@ pub(crate) struct Config {
     pub(crate) max_concurrent_streams: Option<u32>,
     pub(crate) max_pending_accept_reset_streams: Option<usize>,
     pub(crate) max_local_error_reset_streams: Option<usize>,
-    #[cfg(feature = "runtime")]
     pub(crate) keep_alive_interval: Option<Duration>,
-    #[cfg(feature = "runtime")]
     pub(crate) keep_alive_timeout: Duration,
     pub(crate) max_send_buffer_size: usize,
     pub(crate) max_header_list_size: u32,
+    pub(crate) date_header: bool,
 }

the runtime feature flag has since been removed. see: https://docs.rs/hyper/1.5.1/hyper/#optional-features

the backported code, consequently, does not include these conditional compilation attributes. i was able to recreate this as far back as v0.14.25, when these backported types were first released.

this commit proposes the addition of #[cfg(feature = "runtime")] attributes to the keep_alive_timeout(..) and keep_alive_interval(..) methods of the backported http/2 server connection Builder.

this will allow hyper users that only require the server and http2 feature flags to address deprecations and prepare to upgrade to hyper 1.0, without having to opt into previously disabled feature flags.

currently, dependents of hyper 0.14 that enable the `server` and `http2`
feature flags will see an error if they enable the `backports` flag.

building hyper like so...

```sh
cargo check --features server,http2,backports
```

...will yield the following errors (shown in "short" format here for
brevity):

```text
src/server/conn.rs:75:9: warning: unused import: `tracing::trace`
src/server/conn/http2.rs:185:25: error[E0609]: no field `keep_alive_interval` on type `h2::server::Config`
src/server/conn/http2.rs:199:25: error[E0609]: no field `keep_alive_timeout` on type `h2::server::Config`
warning: `hyper` (lib) generated 1 warning
error: could not compile `hyper` (lib) due to 2 previous errors; 1 warning emitted
```

this stems from the fact that the deprecated connection builder
`hyper::server::conn::Builder` and the backported
`hyper::server::conn::http2::Builder` both operate on the same internal
`Config` structure. this structure has changed slightly between 0.14 and
1.0 however. see:

```sh
; git diff 0.14.x master -- src/proto/h2/server.rs | grep 'struct Config' -A 11
```

```diff
@@ -51,12 +51,11 @@ pub(crate) struct Config {
     pub(crate) max_concurrent_streams: Option<u32>,
     pub(crate) max_pending_accept_reset_streams: Option<usize>,
     pub(crate) max_local_error_reset_streams: Option<usize>,
-    #[cfg(feature = "runtime")]
     pub(crate) keep_alive_interval: Option<Duration>,
-    #[cfg(feature = "runtime")]
     pub(crate) keep_alive_timeout: Duration,
     pub(crate) max_send_buffer_size: usize,
     pub(crate) max_header_list_size: u32,
+    pub(crate) date_header: bool,
 }
```

the `runtime` feature flag has seen been removed. see:
<https://docs.rs/hyper/1.5.1/hyper/#optional-features>

the backported code, consequently, does not include these conditional
compilation attributes. i was able to recreate this as far back as
v0.14.25, when these backported types were first released.

this commit proposes the addition of `#[cfg(feature = "runtime")]`
attributes to the `keep_alive_timeout(..)` and `keep_alive_interval(..)`
methods of the backported http/2 server connection `Builder`.

this will allow hyper users that only require the `server` and `http2`
feature flags to address deprecations and prepare to upgrade to hyper
1.0, _without_ having to opt into previously disabled feature flags.

Signed-off-by: katelyn martin <[email protected]>
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 the write-up and fix!

@seanmonstar seanmonstar merged commit 72ebcff into hyperium:0.14.x Dec 18, 2024
19 checks passed
@cratelyn cratelyn deleted the kate/http2-server-backport-runtime-cfg branch December 18, 2024 21:13
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