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

Websocket support #513

Closed
wants to merge 2 commits into from
Closed

Websocket support #513

wants to merge 2 commits into from

Conversation

brimworks
Copy link
Collaborator

@brimworks brimworks commented Jul 9, 2021

This is the simplest possible implementation of websockets with a net increase in code coverage.

  • No alterations to DataPort.
  • Two new options, one to support httpInterceptors and another to support proxy requests.
  • Websockets is supported via a new schema ("ws" or "wss").
  • Copy writer headers are at the top of every new file added.
  • All websocket implementation details are encapsulated by extending Socket and implementing WebSocket with a WebSocketInputStream and WebSocketOutputStream.

I'm planning to use these changes in our production environment within the next few weeks.

Being critical of myself, these are the points that are lacking:

  • Proper websocket closure handshake is not implemented, close() simply forwards to the underlying socket.
  • The http interceptors don't allow filtering HTTP responses and are very primitive. They work for my use case, but not sure if it works for all use-cases.
  • The handling of upgradeToSecure is a little awkward since websockets is typically performed over a TLS channel and thus there is no need for double encryption. A better solution would involve delegating this to the DataPort... or more specifically, when the secure() option is set, we should NOT throw an error if "wss" transport is used.
  • Create URI for server now takes in an "is websocket" parameter, again this is a little awkward. A better solution would involve delegating this to the DataPort.
  • No support for Sec-WebSocket-Extensions: permessage-deflate (aka websocket level compression... which should really be combined with Nats-No-Masking: TRUE to really get the full benefits).
  • The implementation of websocket framing strikes a balance between TCP fragmentation and extra buffer copying by using a 1440 sized buffer used for the initial frame header + the first ~1438 bytes. The channel based implementation mitigates this fragmentation by using the GatheringByteChannel and passing in an array of ByteBuffers.

Note that this PR replaces #426 which was posted before I had access to the nats-io/nats.java repository (and for some reason that other PR isn't working with coveralls).

For more information on websockets, I'd really recommend reading the RFC: https://datatracker.ietf.org/doc/html/rfc6455

A 4x more complex PR which involves a DataPort refactor was posted here:

I've closed out the 4x more complex PRs, since the guiding principle that @scottf suggests is simplicity... and this is as simple as it gets.

@brimworks brimworks force-pushed the websocket-support branch 4 times, most recently from 653ebbb to 5a118fe Compare July 9, 2021 21:04
@brimworks brimworks requested a review from scottf July 9, 2021 21:34
@brimworks
Copy link
Collaborator Author

The only issue with this so far is in regards to using proxies that require authentication. If a proxy requires authentication, you can call Authenticator.setDefault(), and override the getPasswordAuthentication() to return the username and password to use in the proxy.

However, this is also effectively a global variable, and thus impacts any usage of Proxy global to the JVM.

To mitigate this, we could define an option used to construct a new Socket object (effectively a socket factory). If that option was available, one could define a socket factory that creates a TCP socket and then connects to the proxy using CONNECT and the appropriate Authorization header. Note that the "real" hostname + port that we are trying to connect to would need to be provided as inputs to this socket factory.

Brian Maher added 2 commits November 11, 2021 14:18
…called which was caused by the websocket "close body" to be returned from the input stream.
@RichardHightower
Copy link
Contributor

Tried to review this. Checked out the branch, and the WebSocket tests were failing. There were 13 WebSocket tests that are failing.

@brimworks
Copy link
Collaborator Author

Hi @RichardHightower what tests where failing? Last time I checked out the branch the tests succeeded, although some of the tests are unreliable (not due to the websocket code, just in general some of the tests have been unreliable in the past... and I even put together some PRs in the past to fix those).

@scottf
Copy link
Contributor

scottf commented Sep 7, 2022

image

@scottf
Copy link
Contributor

scottf commented Sep 7, 2022

@brimworks We are revisiting this as a review of open PR's with some work we are considering doing to add in Vert-x. I though your interface changes might be re-usable or at least a roadmap. I will try to get to the conflicts, they look to be relatively minor.

@brimworks
Copy link
Collaborator Author

Closing this in favor of #826

...as that seems to be the path forward. Thanks for moving this along!

@brimworks brimworks closed this Jan 7, 2023
@scottf scottf deleted the websocket-support branch March 20, 2023 14:28
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

3 participants