actix / actix-extras

A collection of additional crates supporting the actix and actix-web frameworks.
https://actix.rs
Apache License 2.0
785 stars 206 forks source link

actix-ws should support `Sec-WebSocket-Protocol` (or explain why it doesn't) #479

Open sjoqvist opened 3 weeks ago

sjoqvist commented 3 weeks ago

Sub-protocols are a part of the WebSocket specification (see RFC 6455):

The |Sec-WebSocket-Protocol| header field is used in the WebSocket
opening handshake.  It is sent from the client to the server and back
from the server to the client to confirm the subprotocol of the
connection.  This enables scripts to both select a subprotocol and be
sure that the server agreed to serve that subprotocol.

They also seem to have been supported by actix-web-actors: https://github.com/actix/actix-web/blob/568bffeb58db083d63d887bd0f1cceaa0464d1c9/actix-web-actors/src/ws.rs#L425-L448

However, actix-web-actors has been deprecated in favor of actix-ws, which doesn't appear to support sub-protocols. I can't find any references to it, so I don't know if it's intentionally left out, and in that case intended or not intended for later implementation. If it's intentionally left out without the intention of implementing it later, it would save the user's time if this is mentioned in the documentation.

I'd recommend either

  1. allowing sub-protocol negotiation, for example using function parameters, or
  2. explaining the design decision to not include it.
asonix commented 2 weeks ago

I see no reason not to support sub protocols in actix-ws. The reasoning is when I first built actix-ws it was more of a "proof you can do websockets in actix-web without actors" and it hasn't changed much since it was adopted in actix-extras.

I think this request along with this other PR https://github.com/actix/actix-web/pull/3496 deserve some extra thought, though. I feel like there should be a way to hook into actix-ws' handshake and encoding steps without needing to open PRs to actix-http and actix-web for every configuration (I may be proposing a new middleware system for websockets)

I would love to hear some thoughts on this from @robjtede and maybe @segfault87

segfault87 commented 2 weeks ago

When it comes to https://github.com/actix/actix-web/pull/3496, it looks like is in a little bit different situation becuase it is an extension, not a subprotocol. Although it's nothing with subprotocols, I'll try to give my thoughts on the concept of "middleware"s.

The problem with current actix-http/ws is it conceals every inner details of the payload, rendering implementing extensions outside of actix-http nearly impossible. In order to support provide APIs for middlewares, lower-level side of WS frames should be exposed to the API surface.

And it raises some questions:

Extensions are extensions, and it would be nice if they could be provided outside of the core library. Sadly there's no infrastructure for that and implementing it would bring up another long processes of discussions and API changes. I needed compression feature as soon as possible and that's why I implemented the feature in actix-http. I want https://github.com/actix/actix-web/pull/3496 to keep in current form for now but I'm open to discussion. If there are settled API design around middlewares, I'll happily move to the new API.