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

Add TLSSocket.applicationProtocolOption and better docs for TLSSocket.applicationProtocol #3215

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

arturaz
Copy link
Contributor

@arturaz arturaz commented Apr 26, 2023

Copy link
Member

@armanbilge armanbilge left a 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.

Comment on lines 40 to 47
/** 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]
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 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.

Copy link
Contributor Author

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.
Copy link
Member

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]]
Copy link
Member

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?).

Copy link
Contributor Author

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 :)

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.

None yet

2 participants