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

Support multiple subprotocols in Websocket handshake for client side #2607

Merged

Conversation

RomanBrodetski
Copy link
Contributor

Currently passing multiple websockets subprotocols in Sec-WebSocket-Protocol header is not supported (see #2606). This PR makes it possible to do so, passing them separated by comma in subprotocol: Option[String] field of the WebSocketRequest model. This corresponds to the Sec-WebSocket-Protocol header definition in RFC 6455

A better solution would be to change the WebSocketRequest model to accept a subprotocols: Seq[String] field, but that would break compatibility with current client code. If it, nevertheless, makes sense to do this change, please let us know - we are happy to submit a corresponding PR.

References #2606

@lightbend-cla-validator

Hi @RomanBrodetski,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@akka-ci
Copy link

akka-ci commented Jul 19, 2019

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@raboof
Copy link
Member

raboof commented Jul 22, 2019

OK TO TEST

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Jul 22, 2019
@akka-ci
Copy link

akka-ci commented Jul 22, 2019

Test FAILed.

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Looks useful!

@raboof
Copy link
Member

raboof commented Jul 22, 2019

(failure was unrelated, #2349)

@raboof
Copy link
Member

raboof commented Jul 22, 2019

PLS BUILD

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Jul 22, 2019
@akka-ci
Copy link

akka-ci commented Jul 22, 2019

Test PASSed.

@jrudolph jrudolph changed the title Support multiple subprotocols in Websocket handshake Support multiple subprotocols in Websocket handshake for client side Jul 23, 2019
Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

The clean way would be to change WebSocketRequest to support a list of subprotocols directly, however, that might be hard to achieve binary-compatibly.

Could you at least add another apply method to WebSocketRequest that takes a subprotocols: Seq[String] instead of an subprotocol: Option[String], so that this can become the official way of specifying multiple subprotocols?

Co-authored-by: Anton Onyshchenko <anton.onishchenko@gmail.com>
@RomanBrodetski RomanBrodetski force-pushed the websocket-multiple-subprotocols branch from ad6d1a3 to e482c53 Compare July 23, 2019 09:28
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Jul 23, 2019
@akka-ci
Copy link

akka-ci commented Jul 23, 2019

Test PASSed.

Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

Thanks @RomanBrodetski. LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants