Implement WebSocket support based on Plug.Conn.Adapter.upgrade/3

[mtrudel]

This PR provides support for upgrading HTTP connections to WebSocket connections by use of the c:Plug.Conn.Adapter.upgrade/3 callback. WebSocket implementations are called the :cowboy_websocket API. This work is hoisted in large part directly from Phoenix's Phoenix.Endpoint.Cowboy2Handler. This work is the Plug.Cowboy analog of https://github.com/mtrudel/bandit/pull/38

See https://github.com/phoenixframework/phoenix/issues/5003 for an omnibus overview of the larger body of work that this PR is part of.

[mtrudel]

@josevalim as requested, this work is done and ready for review! See https://github.com/phoenixframework/phoenix/issues/5003 for an overview & merge proposal.

[josevalim]

Hi @mtrudel!

Thanks for the PRs, I have started reviewing them and I will drop some feedback around. It may look a bit fragmented but we can go back to the Omnissue if necessary.

For this PR, I want to note that Cowboy will not expose the Sock API. The point is exactly that each webserver has a different API and unifying them can lead us to an unhealthy common ground, especially given Bandit is more expressive than Cowboy.

Therefore, the goal in adapter_upgrade(conn, :websockets, ARG) is for ARG to be anything adapter specific and the behaviour will be dictated by the adapter. Then Sock will implement the callbacks on top of each webserver API (which is what Phoenix does today),

[josevalim]

To further clarify, for Cowboy the API will be:

adapter_upgrade(conn, :websockets, {handler, arg, opts})

Where handler will literally be the Cowboy handler, with no wrapping or indirection whatsoever. Cowboy will call the handler directly.

[mtrudel]

To further clarify, for Cowboy the API will be:

adapter_upgrade(conn, :websockets, {handler, arg, opts})

Where handler will literally be the Cowboy handler, with no wrapping or indirection whatsoever. Cowboy will call the handler directly.

@josevalim I've implemented the required changes to Plug.Cowboy to move in this direction, though it's not ending up as clean as we were hoping. As a result, I haven't pushed them to this branch since I'm not sure if this is an avenue we'll want to end up pursuing. You can see the changes at https://github.com/elixir-plug/plug_cowboy/compare/master...mtrudel:plug_cowboy:plug_upgrade_no_sock

The primary problem is that we're not able to get Cowboy to call the handler directly; from the perspective of Cowboy, Plug.Cowboy.Handler is the handler, and I don't see a way of changing Cowboy's perspective on that. As a result, you will see that we still need to thinly wrap [1] whatever handler we're upgrading to.

Moreover, to me this really starts to muddle the waters about the overall structure of the stack.

If we're still having to do some sort of wrapping/adapting in here I don't really see the benefit to this change; it's just adding another layer of indirection of the exact kind that we iterated on removing in the earlier incarnations of Sock within Phoenix. Instead of having Plug.Cowboy 'pass through' the complexity of :cowboy_websocket, we end up with two layers in the stack that need to understand :cowboy_websocket, which seems like one too many.

Taking a step back, perhaps this would hang together more cleanly with @ericmj's suggestion above, namely moving the Sock behaviour into Plug as Plug.WebSocket and broadening the purview of the Plug library to encompass WebSocket connections as well (we'd talked about this near the start of this whole workup).

Graphically, we'd be looking at something like the second option below:

Cowboy <--:cowboy_websocket--> Plug.Cowboy <--:cowboy_websocket--> Sock <--Sock API--> Phoenix [2]
vs. 
Cowboy <--:cowboy_websocket--> Plug.Cowboy <--Plug.WebSocket--> Phoenix

The benefit of the second approach is that the Sock library doesn't even need to exist at all! Less code & fewer abstractions all around, and a stackup that's consistent with the HTTP side of things. [3]

Really, I think it comes down to your concern that '[each] webserver has a different API and unifying them can lead us to an unhealthy common ground', and where that gets expressed in the stack. The unification of disparate server capabilities under a common Sock/Plug.WebSocket API has to happen somewhere in the stack, and having it at the adapter boundary has worked very well for Plug in the HTTP world (it's certainly the North Star that I've been building Bandit towards as a 'Plug/Sock native' implementation).

In any case, this is of course a PR against your library and thus this is obviously your call :). The changes on both this PR and the branch above are complete viz a viz tests / docs / &c so whichever direction you want to go, the work there is solid and ready to go pending merge of https://github.com/elixir-plug/plug/pull/1119 and and update to the dependencies here.

Also, thanks once again for such timely and thoughtful reviews!

[1] Of note, this wrapping is a clean passthrough of the plain :cowboy_websocket behaviour, beyond the bit of mess to track its state via a tuple in our state & having to check for optional callbacks.

[2] This is the long-term plan. In the near term the Sock steps aren't present, but the fact remains that multiple layers in the stack still need to understand the :cowboy_websocket abstraction stands.

[3] This does preclude splitting phoenixframework/phoenix#5030 into two separate workups. If we went this way we'd also probably want to revisit the _adapter suffix on Plug.Conn.upgrade_adapter/3.

[josevalim]

I see @mtrudel, thanks for exploring! Maybe it is worth sending a pull request to Cowboy, to additionally support {cowboy_websocket, req, handler, state, opts}, so we could also change the handler? Maybe it is something we could do ourselves if the protocol layer in Cowboy is pluggable?

[josevalim]

Looking at the Cowboy code, I believe we could easily do it!

https://github.com/ninenines/cowboy/blob/master/src/cowboy_handler.erl#L41-L44

We just need to define a upgrade/5 function in a module somewhere, use that module as the first tuple element, and then delegate to cowboy_websocket passing the new handler.

[mtrudel]

Looking at the Cowboy code, I believe we could easily do it!

https://github.com/ninenines/cowboy/blob/master/src/cowboy_handler.erl#L41-L44

We just need to define a upgrade/5 function in a module somewhere, use that module as the first tuple element, and then delegate to cowboy_websocket passing the new handler.

I agree. This looks like an easy workup that we can do outside of Cowboy. I'm not sure if the relevant calls in cowboy_websocket would be considered private, however (they're exported, but not documented).

[josevalim]

upgrade/5 is part of the contract and cosboy_websocket is explicitly part of the contract as we can return it, so I think we are good!

[mtrudel]

Very good!

[mtrudel]

The proposed upgrade/5 approach works as expected, and has been updated here. All tests / local validation is green.

A couple of things to note:

• Because plug_cowboy no longer contains any code that runs within the websocket process (ie: within c:cowboy_websocket.websocket_init/1), it can no longer support the fullsweep_after option as passed in from Phoenix. Wiring this up will have to once again become a Phoenix responsibility in phoenixframework/phoenix#5030
• We don't fully support passing through the :cowboy_websocket behaviour. Specifically, we don't support passing through c:cowboy_websocket.init/2 to the handler module. This is a bit of a conceptual discontinuity

Otherwise this should be good to go!

[josevalim]

Because plug_cowboy no longer contains any code that runs within the websocket process (ie: within c:cowboy_websocket.websocket_init/1), it can no longer support the fullsweep_after option as passed in from Phoenix.

It actually has to be. fullsweep_after must be called in the socket process and we were calling it earlier on by mistake (where it did not have much of an effect).

We don't fully support passing through the :cowboy_websocket behaviour. Specifically, we don't support passing through c:cowboy_websocket.init/2 to the handler module. This is a bit of a conceptual discontinuity

To be clear, do you mean that we support all callbacks, except init? I think that's expected, because the init has already been "called" by Plug.

[josevalim]

We are almost there! Just three minor comments left!

[mtrudel]

Because plug_cowboy no longer contains any code that runs within the websocket process (ie: within c:cowboy_websocket.websocket_init/1), it can no longer support the fullsweep_after option as passed in from Phoenix.

It actually has to be. fullsweep_after must be called in the socket process and we were calling it earlier on by mistake (where it did not have much of an effect).

Yeah, I'm talking about after your recent fix to that in phoenixframework/phoenix#4933. In any case, it'll be in the right place now :)

We don't fully support passing through the :cowboy_websocket behaviour. Specifically, we don't support passing through c:cowboy_websocket.init/2 to the handler module. This is a bit of a conceptual discontinuity

To be clear, do you mean that we support all callbacks, except init? I think that's expected, because the init has already been "called" by Plug.

Yes, exactly.

[mtrudel]

Other than a final change to the plug dependency herein, all done!

[mtrudel]

PR updated to depend on 1.14.0. Note that this brings up a number of deprecation errors around push (what's happening with that, btw? I removed push support in bandit recently as a follow-on to it being deprecated in Plug).

Testing green locally; assume it will on CI now as well!

[josevalim]

what's happening with that, btw?

https://developer.chrome.com/blog/removing-push/

[mtrudel]

what's happening with that, btw?

https://developer.chrome.com/blog/removing-push/

The funny thing is that push was probably the most difficult thing to get right in Bandit's HTTP/2 implementation. Had I not been such a completionist and skipped over it, I'd have saved a ton of time. Oh well 🙃

[josevalim]

Can you please update CI to use Elixir v1.10 and update the mix.exs to require ~> 1.10 (the same as Plug)?

[mtrudel]

Yep, just saw that too. Coming right up

[mtrudel]

Updated to the same min OTP/Elixir pair as Plug. Note that:

• OTP actually went down to 21.3
• I didn't touch the third 1.11.4/23.2.7 pair in the matrix

Let's see how CI likes that.

[josevalim]

OTP actually went down to 21.3

Cowboy requires 22 IIRC.

[mtrudel]

Updated to just change Elixir to 1.10, leaving OTP unchanged

[josevalim]

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart:

[mtrudel]

🥳