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.

Sign up for GitHub

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

Connector: enable_all_versions with matching alpn vs features #224

Merged
merged 2 commits into from
Oct 5, 2023
Merged

Connector: enable_all_versions with matching alpn vs features #224

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
191d8bd
connector: `enable_all_versions` with matching alpn vs features
TimLuq Oct 3, 2023
0b4b6e1
connector: add `http2`-only alpn tests
TimLuq Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions src/connector/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,16 +174,22 @@ impl ConnectorBuilder<WantsProtocols1> {
})
}

/// Enable all HTTP versions
/// Enable all HTTP versions built into this library
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this more explicit, like (enabled with Cargo features)?

///
/// For now, this enables both HTTP 1 and 2. In the future, other supported versions
/// will be enabled as well.
#[cfg(all(feature = "http1", feature = "http2"))]
/// For now, this could enable both HTTP 1 and 2, depending on active features.
/// In the future, other supported versions will be enabled as well.
#[cfg(feature = "http2")]
#[cfg_attr(docsrs, doc(cfg(feature = "http2")))]
Comment on lines +181 to +182
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to do this, let's get rid of the http2 guard? And make it work for all of the possible combinations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that seems like the right direction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djc

The problem with not having the http2 guard is that the return type would change (since WantsProtocols3 wont exist). So if any library enables the feature it might cause other libraries to not compile if they expected WantsProtocols2. Since enabling an extra feature in a deep dependency doesn't usually warrant a major version release, things in the wild could spontaneously break if one single library enables it when one other doesn't expect it.

Making the feature combination non-breaking would require wrapping the actual inner type WantsProtocolsN (based on the highest enabled protocol feature) in a WantsProtocolsAll singleton struct and always returning that. That would be a breaking change for this library, however, which is why I didn't want to do it in this fix.

If a version of this fix gets merged I could create a new PR with the above solution with a new breaking return type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems reasonable, I'd be willing to merge this in order to get this improvement out provided you submit an additional PR to do the semver-breaking fix.

pub fn enable_all_versions(mut self) -> ConnectorBuilder<WantsProtocols3> {
self.0.tls_config.alpn_protocols = vec![b"h2".to_vec()];
#[cfg(feature = "http1")]
let alpn_protocols = vec![b"h2".to_vec(), b"http/1.1".to_vec()];
#[cfg(not(feature = "http1"))]
let alpn_protocols = vec![b"h2".to_vec()];

self.0.tls_config.alpn_protocols = alpn_protocols;
ConnectorBuilder(WantsProtocols3 {
inner: self.0,
enable_http1: true,
enable_http1: cfg!(feature = "http1"),
})
}

Expand Down Expand Up @@ -326,7 +332,7 @@ mod tests {
.build();
assert_eq!(&connector.tls_config.alpn_protocols, &[b"h2".to_vec()]);
let connector = super::ConnectorBuilder::new()
.with_tls_config(tls_config)
.with_tls_config(tls_config.clone())
.https_only()
.enable_http1()
.enable_http2()
Expand All @@ -335,5 +341,14 @@ mod tests {
&connector.tls_config.alpn_protocols,
&[b"h2".to_vec(), b"http/1.1".to_vec()]
);
let connector = super::ConnectorBuilder::new()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is only checking the result of using enable_all_versions with both the http1 and http2 feature flags enabled based on the guard on test_alpn. Ideally we could assert the correct state for more combinations than just this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we could assert the correct state for more combinations than just this case.

Yeah, I realized this too. I locally created an additional test case for only http2 while I was writing the PR description. I suspected that I'd get feedback on the doc line so I didn't want to muddy the waters with an additional small commit. But I'll push it now.

.with_tls_config(tls_config)
.https_only()
.enable_all_versions()
.build();
assert_eq!(
&connector.tls_config.alpn_protocols,
&[b"h2".to_vec(), b"http/1.1".to_vec()]
);
}
}