-
Notifications
You must be signed in to change notification settings - Fork 603
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
Add TLSSocket.applicationProtocolOption
and better docs for TLSSocket.applicationProtocol
#3215
base: main
Are you sure you want to change the base?
Conversation
…ket.applicationProtocol`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not too hard would be good to add some tests for this. It might be straightforward to call applicationProtocol
in our existing tests and check its value.
/** Provides access to the current application protocol that has been negotiated. | ||
* | ||
* Raises a [[NoSuchElementException]] if it has not yet been determined if application protocols might | ||
* be used for this connection. See [[applicationProtocolOption]] for a safer option. | ||
* | ||
* @see https://discord.com/channels/632277896739946517/632310980449402880/1100649913223745597 | ||
*/ | ||
def applicationProtocol: F[String] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can even deprecate this API to encourage the other one :)
Not sure about linking to Discord. We should record anything important either in discussion on this PR or in code / comments themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience linking to Discord gives the perspective on back-and-forth that we had until we arrived at this. It's not super important, but it might give some insight into the context for a person looking at it.
*/ | ||
def applicationProtocol: F[String] | ||
|
||
/** Provides access to the current application protocol that has been negotiated. | ||
* | ||
* Returns [[None]] if it has not yet been determined if application protocols might be used for this connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should include the bit about the empty String
? Actually, this almost deserves its own ADT but I'm not sure if it's worth it 🤔 or, maybe that case should be mapped to None
as well? I'm not sure if there is a meaningful distinction.
null if it has not yet been determined if application protocols might be used for this connection, an empty String if application protocols values will not be used, or a non-empty application protocol String if a value was successfully negotiated.
def applicationProtocol: F[String] | ||
/** Returns [[None]] if there is no protocol being negotiated. | ||
* | ||
* @see [[https://aws.github.io/s2n-tls/doxygen/s2n_8h.html#ae53faa26669e258afff875d45140f14e]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc says:
In the event of no protocol being negotiated, NULL is returned.
Is this actually equivalent to the empty string ""
case on the JVM? I'm not sure. On the JVM None
is if it's not yet determined, but ""
is if it's not being used (not being negotiated?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am even less qualified to answer this question, not familiar with TLS enough :)
See https://discord.com/channels/632277896739946517/632310980449402880/1100649913223745597