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 server compression does not work #494

Closed
favila opened this issue Mar 9, 2019 · 3 comments
Closed

websocket server compression does not work #494

favila opened this issue Mar 9, 2019 · 3 comments

Comments

@favila
Copy link

favila commented Mar 9, 2019

Websocket servers never actually upgrade to use compression, so the :compression? option on websocket-connection actually does nothing.

I've verified this by noting the returned headers in Chrome dev tools: Chrome asks for permessage-deflate, but the return headers never contain Sec-WebSocket-Extensions: permessage-deflate.
I've also added breakpoints (in a java debugger) to the points where extension detection should happen, and those breakpoints are never reached.
Unfortunately I don't know how to test this using the public aleph api; I don't know how to write a aleph.websocket-test test that actually confirms compression is on after connection upgrade, from either the client or server side.

The problem is that the WebSocketServerCompressionHandler added to the pipeline uses a WebSocketServerExtensionHandler which turns extensions on by examining the channelRead event for a HttpRequest message. However, this method never sees any HttpRequest messages. The first message it will see is an actual websocket message created by the netty/channel-inbound-handler created in aleph.http.server/websocket-server-handler.

I am a netty novice so I don't know how to fix this. This might be netty rather than an aleph bug (although I couldn't find any netty tickets that looked like this). It seems like the handshaker has an opportunity to re-fire the httprequest as a channel read, but that would require changing netty (I think) and I don't know if it's correct. An alternative is maybe to do the compression extension sniffing somewhere inside initialize-websocket-handler (i.e. reimplement most of WebsocketServerExtensionHandler.channelRead()) and add the appropriate compression handler to the pipeline directly (i.e. reimplement WebsocketServerExtensionHandler.write().)

@kachayev
Copy link
Collaborator

kachayev commented Mar 9, 2019

@favila Good catch. I'm working on a WebSocket handling flow right now (mostly due to potential issues with leaked buffers). I've spotted the same issue, to make this work with need WebSocket*ExtensionHandshaker to be executed during the handshake and rsv with extensions bit to be passed through frames. Which is not trivial to handle correctly, but I'm sure I'll get there soon. In terms of testing, we can try to leverage autobahn project, at least the description tells that per-message deflate test is included.

@kachayev
Copy link
Collaborator

kachayev commented Mar 22, 2019

@favila This actually happens because we misuse WebsocketServerCompressionHandler. The idea behind Aleph's WebSocket server is that you can take any HTTP request that you already have and "upgrade" the connection to WebSocket. It gives you a lot of flexibility, e.g. you can work with your own routing to decide which route handles WebSockets etc. But the key problem here: we're getting HTTP request from a generic HTTP server pipeline to use it to setup handshaker... and only after that we actually rebuild the pipeline with the handler mentioned. WebsocketServerCompressionHandler is implemented by defining channelRead, so it actually waits for the request to be read, that specific request that we've already read to decide if we need WebSocket or not.

There's no simple solution here, I would probably do the following:

  1. Reimplement WebsocketServerCompressionHandler based on Aleph's request/response flow
  2. Submit PR to Netty with the same functionality.
  3. Extend handshake processing to attach a description to Manifold's stream (the same way it works for closing handshake) with proper information from HandshakeComplete event, so we can test this functionality carefully.

Stay tuned and thanks for the reporting.

@kachayev
Copy link
Collaborator

The solution was already merged, closing the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants