Add a utility that checks if SslProvider.JDK supports ALPN#9693
Merged
normanmaurer merged 1 commit intonetty:4.1from Oct 23, 2019
Merged
Add a utility that checks if SslProvider.JDK supports ALPN#9693normanmaurer merged 1 commit intonetty:4.1from
normanmaurer merged 1 commit intonetty:4.1from
Conversation
|
Can one of the admins verify this patch? |
Member
|
@idelpivnitskiy after thinking a bit more about it I wonder if we not just want to add a method like this to SslContext and so make things easier for users to discover in the first place: |
Member
|
@idelpivnitskiy let me know what you think... |
Member
Author
|
@normanmaurer agreed, discoverability will be better if we add a static method to Additional questions to discuss in this case:
|
Member
|
Sounds like a plan 👌
… Am 21.10.2019 um 19:44 schrieb Idel Pivnitskiy ***@***.***>:
@normanmaurer agreed, discoverability will be better if we add a static method to SslContext class. However, SslContext has a lot of deprecated static methods. Not sure if we want to add a new one to this abstract class.
Alternative could be to add a similar static (or non-static) method directly to the SslProvider enum. That will help to avoid forgetting to update this method in case we add a new SslProvider and also good for discoverability, because the check is related to SslProvider.
Additional questions to discuss in this case:
Should we deprecate OpenSsl.isAlpnSupported() and suggest to use a new method?
Should we also add SslProvider.default() (or SslProvider.defaultForClient()/SslProvider.defaultForServer()) and deprecate SslContext.defaultClientProvider()/SslProvider.defaultServerProvider()? That will help to remove all public static methods from SslContext in the 5.0 version.
WDYT?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Motivation: We have a public utility `OpenSsl.isAlpnSupported()` that helps users to check if ALPN is available for `SslProvider.OPENSSL`. However, we do not provide a similar utility for `SslProvider.JDK`. Therefore, users who configured ALPN with `SslProvider.JDK` will get a runtime exception at the time when a new connection will be created. Modifications: - Add public `SslProvider.isAlpnSupported(SslProvider)` utility method that returns `true` if the `SslProvider` supports ALPN; - Deprecate `OpenSsl.isAlpnSupported()`; Result: Users can verify if their environment supports ALPN with `SslProvider.JDK` upfront (at bootstrap), instead of failing with runtime exception when a new connection will be created.
b7ed2e7 to
2426b09
Compare
Member
Author
|
@normanmaurer pushed the fix and updated commit message/PR description. |
Member
|
@netty-bot test this please |
normanmaurer
approved these changes
Oct 22, 2019
Member
|
@idelpivnitskiy thanks a lot! |
normanmaurer
pushed a commit
that referenced
this pull request
Oct 23, 2019
Motivation: We have a public utility `OpenSsl.isAlpnSupported()` that helps users to check if ALPN is available for `SslProvider.OPENSSL`. However, we do not provide a similar utility for `SslProvider.JDK`. Therefore, users who configured ALPN with `SslProvider.JDK` will get a runtime exception at the time when a new connection will be created. Modifications: - Add public `SslProvider.isAlpnSupported(SslProvider)` utility method that returns `true` if the `SslProvider` supports ALPN; - Deprecate `OpenSsl.isAlpnSupported()`; Result: Users can verify if their environment supports ALPN with `SslProvider` upfront (at bootstrap), instead of failing with runtime exception when a new connection will be created.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
We have a public utility
OpenSsl.isAlpnSupported()that helps users tocheck if ALPN is available for
SslProvider.OPENSSL. However, we do notprovide a similar utility for
SslProvider.JDK. Therefore, users whoconfigured ALPN with
SslProvider.JDKwill get a runtime exception atthe time when a new connection will be created.
Modifications:
SslProvider.isAlpnSupported(SslProvider)utility method thatreturns
trueif theSslProvidersupports ALPN;OpenSsl.isAlpnSupported();Result:
Users can verify if their environment supports ALPN with
SslProvider.JDKupfront (at bootstrap), instead of failing with runtimeexception when a new connection will be created.