Connector: enable_all_versions with matching alpn vs features
#224
Conversation
src/connector/builder.rs
Outdated
| @@ -174,16 +174,22 @@ impl ConnectorBuilder<WantsProtocols1> { | |||
| }) | |||
| } | |||
|
|
|||
| /// Enable all HTTP versions | |||
| /// Enable all HTTP versions built into this library | |||
There was a problem hiding this comment.
Let's make this more explicit, like (enabled with Cargo features)?
| #[cfg(feature = "http2")] | ||
| #[cfg_attr(docsrs, doc(cfg(feature = "http2")))] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Agreed, that seems like the right direction
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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() | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| #[cfg(feature = "http2")] | ||
| #[cfg_attr(docsrs, doc(cfg(feature = "http2")))] |
There was a problem hiding this comment.
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.
|
Thanks! |
|
+1, thank you |
Fixes #208
The original intent of this function is slightly confusing. As it was implemented before, it required all supported protocol features to be enabled for the function to exist (both
http1andhttp2). My naive guess would have been that the expected behavior is that all enabled protocol features would inform the actual functionality, but the function would still show up if at least one protocol is enabled.I understand that having this completely dynamic based on active features would muddle the documented return type and be breaking depending on active features. Either way, to me, it hurt a bit working on a function named
enable_all_versionswith a strict feature requirement on the non-default optional featurehttp2. But now it should work correctly in the previously intended use cases.Changes to
enable_all_versionshttp1featurehttp2to fn docs (previously a non-documented requirement)h2ifhttp1is not enabledh2andhttp/1.1if both features are enabled