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

mtrudel · 2022-10-03T23:09:14Z

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 mtrudel/bandit#38

See phoenixframework/phoenix#5003 for an omnibus overview of the larger body of work that this PR is part of.

mtrudel · 2022-10-16T20:17:01Z

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

lib/plug/cowboy/conn.ex

ericmj · 2022-10-29T14:07:03Z

mix.exs

@@ -34,6 +34,7 @@ defmodule Plug.Cowboy.MixProject do
  def deps do
    [
      {:plug, "~> 1.7"},
+      {:sock, "~> 0.3.0"},


What do you think of making this part of plug as Plug.Websocket instead? Not a fan of adding a dependency that is just a single behaviour when we already have plug for HTTP interfaces.

Yes, I dropped a comment. We should not depend on Sock because we should not expose the Sock API but the Cowboy API. Then Sock sits on top!

(I wrote this in the flurry earlier today but it never actually got sent)

~~Got it.~~ Actually, maybe I don't :)

I follow you as far as removing Sock from this PR, and having Plug.Cowboy expose Cowboy WS handler API. That much is clear.

When you say 'Sock sits on top!', are you suggesting that a caller does something like:

def call(conn, opts) do .... # Assuming that MyApp.Socket is what you ultimately want to handle the WS upgrade_adapter(conn, :websocket, {Sock, {MyApp.Socket, opts}, connection_opts}) end

At which time the underlying server (say Cowboy) will call Sock via the Cowboy WS API, and Sock will then call onwards to MyApp.Socket via the Sock API?

Yes, the goal is to have Sock.upgrade(conn, sock_options) and that will call upgrade_adapter for each adapter accordingly.

Got it; Sock in this case would wrap the socket lifecycle starting from upgrade.

Only thing I'm not sure is how that would look in the first PR split from phoenixframework/phoenix#5030 before we introduce Sock. Pretty sure it would still need to be cowboy specific.

Yeah, let’s not worry about Sock in that PR yet. Let’s change the plug dispatch bits and I think figuring out the rest will be straightforward!

josevalim · 2022-10-29T14:42:49Z

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 · 2022-10-29T14:44:05Z

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 · 2022-10-29T23:43:18Z

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 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 elixir-plug/plug#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 · 2022-10-30T07:23:02Z

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 · 2022-10-30T08:07:04Z

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 · 2022-10-30T13:27:42Z

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 · 2022-10-30T13:29:55Z

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 · 2022-10-30T13:31:06Z

Very good!

mtrudel · 2022-10-30T15:13:37Z

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 Implement WebSocket negotiation support based on Plug.Conn.upgrade_adapter/3 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 · 2022-10-30T15:44:58Z

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 · 2022-10-30T15:47:20Z

lib/plug/cowboy.ex

+  should handle the WebSocket connection, and must take the form `{handler, handler_opts,
+  connection_opts}`, where values are as follows:
+
+  * `handler` is a module which implements the `:cowboy_websocket` behaviour. Note that this


Let's add a link to cowboy_websocket?

/me looks up how to link to Erlang behaviours in ex_doc lol

It has to be manual:

Suggested change

* `handler` is a module which implements the `:cowboy_websocket` behaviour. Note that this

* `handler` is a module which implements the [`:cowboy_websocket`](https://ninenines.eu/docs/en/cowboy/2.6/manual/cowboy_websocket/) behaviour. Note that this

lib/plug/cowboy.ex

josevalim · 2022-10-30T15:50:10Z

lib/plug/cowboy/conn.ex

@@ -90,6 +90,10 @@ defmodule Plug.Cowboy.Conn do
    :cowboy_req.inform(status, to_headers_map(headers), req)
  end

+  @impl true
+  def upgrade(req, :websocket, args), do: {:ok, Map.put(req, :upgrade, {:websocket, args})}


We should probably validate here that args is a triplet, the first element is an atom and the third is a map.

I was thinking about that at the dog park yesterday - maybe we should add a matching pattern match in the plug PR?

I think it is fine to raise here?

Updated to explicitly match on a three element tuple

josevalim · 2022-10-30T15:50:32Z

We are almost there! Just three minor comments left!

mtrudel · 2022-10-30T15:54:55Z

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 · 2022-10-30T16:05:28Z

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

mtrudel · 2022-10-31T15:39:49Z

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 · 2022-10-31T15:45:06Z

what's happening with that, btw?

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

mtrudel · 2022-10-31T15:47:00Z

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 🙃

lib/plug/cowboy/conn.ex

Co-authored-by: José Valim <jose.valim@gmail.com>

lib/plug/cowboy.ex

Co-authored-by: José Valim <jose.valim@gmail.com>

josevalim · 2022-10-31T15:52:07Z

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

mtrudel · 2022-10-31T15:53:02Z

Yep, just saw that too. Coming right up

mtrudel · 2022-10-31T15:56:20Z

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 · 2022-10-31T16:07:04Z

OTP actually went down to 21.3

Cowboy requires 22 IIRC.

mtrudel · 2022-10-31T16:09:23Z

Updated to just change Elixir to 1.10, leaving OTP unchanged

josevalim · 2022-10-31T16:15:17Z

💚 💙 💜 💛 ❤️

mtrudel · 2022-10-31T16:15:24Z

🥳

This was referenced Oct 3, 2022

Allow Phoenix to provide generic WebSocket support phoenixframework/phoenix#5003

Closed

Add support for protocol upgrades elixir-plug/plug#1119

Merged

Implement no-op support for Plug.Conn.Adapter.upgrade/3 mtrudel/bandit#34

Closed

mtrudel force-pushed the plug_upgrade branch from 4357226 to a14f4e2 Compare October 16, 2022 18:46

mtrudel changed the title ~~Implement no-op support for Plug.Conn.Adapter.upgrade/3~~ Implement WebSocket support based on Plug.Conn.Adapter.upgrade/3 and Sock Oct 16, 2022

mtrudel mentioned this pull request Oct 16, 2022

Implement WebSocket negotiation support based on Plug.Conn.upgrade_adapter/3 phoenixframework/phoenix#5030

Merged

minidfx mentioned this pull request Oct 22, 2022

Websocket support? tallarium/reverse_proxy_plug#56

Open

ericmj reviewed Oct 29, 2022

View reviewed changes

Implement no-op support for Plug.Conn.Adapter.upgrade/3

6d3725d

mtrudel force-pushed the plug_upgrade branch from e71fcb5 to 39fe283 Compare October 29, 2022 23:14

mtrudel force-pushed the plug_upgrade branch from 39fe283 to b040369 Compare October 29, 2022 23:48

mtrudel force-pushed the plug_upgrade branch from b040369 to 9fd8475 Compare October 30, 2022 15:08

mtrudel force-pushed the plug_upgrade branch from 9fd8475 to 3402e4d Compare October 30, 2022 15:15

Implement WebSocket upgrade support

7b918d8

mtrudel force-pushed the plug_upgrade branch from 3402e4d to 7b918d8 Compare October 30, 2022 15:16

josevalim reviewed Oct 30, 2022

View reviewed changes

lib/plug/cowboy.ex Show resolved Hide resolved

josevalim reviewed Oct 30, 2022

View reviewed changes

mtrudel added 3 commits October 30, 2022 11:59

Link to cowboy_websocket behaviour docs

1eb4dd6

Support cowboy connection opts directly

cd892fb

Explicitly match on three element tuple at upgrade time

0da2635

Update plug to 1.14.0

622ed50

mtrudel changed the title ~~Implement WebSocket support based on Plug.Conn.Adapter.upgrade/3 and Sock~~ Implement WebSocket support based on Plug.Conn.Adapter.upgrade/3 Oct 31, 2022

josevalim reviewed Oct 31, 2022

View reviewed changes

lib/plug/cowboy/conn.ex Outdated Show resolved Hide resolved

Explicitly examine upgrade tuple

5526816

Co-authored-by: José Valim <jose.valim@gmail.com>

josevalim reviewed Oct 31, 2022

View reviewed changes

lib/plug/cowboy.ex Outdated Show resolved Hide resolved

Document that we support any / all of Cowboy's WebSocket options

1cdaed8

Co-authored-by: José Valim <jose.valim@gmail.com>

Raise minimum elixir version to 1.10

4bb0af2

mtrudel force-pushed the plug_upgrade branch from ecaa9e7 to 4bb0af2 Compare October 31, 2022 16:08

josevalim merged commit 0227c58 into elixir-plug:master Oct 31, 2022

mtrudel deleted the plug_upgrade branch October 31, 2022 16:15

Gazler mentioned this pull request Sep 6, 2023

:fullsweep_after option is ignored phoenixframework/phoenix#5573

Closed

	* `handler` is a module which implements the `:cowboy_websocket` behaviour. Note that this
	* `handler` is a module which implements the [`:cowboy_websocket`](https://ninenines.eu/docs/en/cowboy/2.6/manual/cowboy_websocket/) behaviour. Note that this

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

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

Conversation

mtrudel commented Oct 3, 2022 • edited

mtrudel commented Oct 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrudel Oct 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Oct 29, 2022

josevalim commented Oct 29, 2022

mtrudel commented Oct 29, 2022 • edited

josevalim commented Oct 30, 2022

josevalim commented Oct 30, 2022

mtrudel commented Oct 30, 2022

josevalim commented Oct 30, 2022

mtrudel commented Oct 30, 2022

mtrudel commented Oct 30, 2022

josevalim commented Oct 30, 2022

Choose a reason for hiding this comment

mtrudel Oct 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Oct 30, 2022

mtrudel commented Oct 30, 2022

mtrudel commented Oct 30, 2022

mtrudel commented Oct 31, 2022 • edited

josevalim commented Oct 31, 2022

mtrudel commented Oct 31, 2022

josevalim commented Oct 31, 2022

mtrudel commented Oct 31, 2022

mtrudel commented Oct 31, 2022 • edited

josevalim commented Oct 31, 2022

mtrudel commented Oct 31, 2022

josevalim commented Oct 31, 2022

mtrudel commented Oct 31, 2022

mtrudel commented Oct 3, 2022 •

edited

mtrudel Oct 30, 2022 •

edited

mtrudel commented Oct 29, 2022 •

edited

mtrudel Oct 30, 2022 •

edited

mtrudel commented Oct 31, 2022 •

edited

mtrudel commented Oct 31, 2022 •

edited