Implement WebSocket negotiation support based on Plug.Conn.upgrade_adapter/3 #5030
Conversation
| try do | ||
| endpoint.call(conn, opts) | ||
| rescue | ||
| exception in [UndefinedFunctionError] -> |
There was a problem hiding this comment.
Note we're rescuing against an UndefinedFunctionError rather than catching a throw as previously. Unsure why that implementation caught instead of rescuing, but this captures the same set of errors.
|
@josevalim as requested, this work is done and ready for review! See #5003 for an overview & merge proposal. |
|
Thanks, I got the PRs. I am swamped with other work but I will review once I have time! |
|
Thank you @mtrudel! I think this looks great but I would break this PR in two. The first PR is to use This in itself will already allow you to plug bandit in, in a transparent way. Then we can have another PR to discuss and move to Sock altogether. :) |
lib/phoenix/endpoint.ex
Outdated
| @@ -909,13 +852,38 @@ defmodule Phoenix.Endpoint do | |||
|
|
|||
| """ | |||
| defmacro socket(path, module, opts \\ []) do | |||
| module = Macro.expand(module, %{__CALLER__ | function: {:__handler__, 2}}) | |||
| module = Macro.expand(module, __CALLER__) | |||
There was a problem hiding this comment.
We need to keep the %{function : ...} bit to avoid compile-time dependencies. If we need to pick another name, it can be call/2, as it is now a regular Plug.
There was a problem hiding this comment.
This is past the limit of my macro brain :) You're saying basically
| module = Macro.expand(module, __CALLER__) | |
| module = Macro.expand(module, %{__CALLER__ | function: {:call, 2}}) |
?
There was a problem hiding this comment.
Yes! Or :socket_dispatch instead of :call if we go down that route. The name doesn't matter.
Very good! |
| @@ -0,0 +1,36 @@ | |||
| defmodule Phoenix.Endpoint.AttemptCodeReloadPlug do | |||
| @moduledoc ~S""" | |||
There was a problem hiding this comment.
Please keep this as @moduledoc false, as it should be private, but keep those notes as code comments!
There was a problem hiding this comment.
Ah, you would need to use this from Bandit? Which then means it has to be public?
|
Branch updated with the first PR of the two-PR split @josevalim requested previously. It's fully functional and testing green. A couple of notes:
Is this what you'd envisioned, @josevalim ? If you're happy with this approach then I think this PR can stand for proper review. |
mtrudel
force-pushed
the
sock_support
branch
3 times, most recently
from
aa6211a
to
12f6a57
Compare
mtrudel
changed the title
| # Cures a Cowboy race condition where it doesn't see our declared websocket_init/1 | ||
| _ = Code.ensure_loaded?(Phoenix.Endpoint.Cowboy2Handler) |
There was a problem hiding this comment.
This was reliably buggy on the server's first WebSocket upgrade without this - switching the handler within Cowboy seems to cause a race with the new handler not being available right away, and so the new handler's (optional!) websocket_init/1 was not being called.
Since this isn't a module that never gets reloaded we're OK having this just load once at startup.
There was a problem hiding this comment.
I can see why this happens: we should fix it in Plug.Cowboy. We should call handler.module_info(:module) before passing it to the websocket upgrade. :)
There was a problem hiding this comment.
I have republished v2.6.0. We should be able to remove this line.
There was a problem hiding this comment.
Confirmed that this fixes the issue. Branch updated
There was a problem hiding this comment.
Just so I understand for next time, does that change fix it due to the side effect of ensuring the module is in fact loaded before calling upgrade/5?
|
mix deps updated for plug 1.14 and plug_cowboy 2.6.0. Should be good to go! |
|
Integration test failure looks to be unrelated |
|
This PR looks great to me. I only need to figure out a way to make the websocket bits inside the WebSocket transport Pluggable. I will tidy it up and I should merge it tomorrow! |
|
If you want to take the option, I've got a good deal of that stuff cleaned up in my 'part 2' PR (still pending). I'm expecting to have it out in the next day or so. It's definitely something that we should look at, but IMO it's strictly beyond the minimal scope of this PR. |
|
Thank you @mtrudel, this is huge! 💚 💙 💜 💛 ❤️ |
|
Hi @mtrudel! I have pushed 08c6e61, which should allow you to inject a upgrade so you can inject I think we want to move the socket connections all the way down to the router. This way we get logging, session handling, and more for free. The same way we just got SSL handling for free. This also means LiveDashboard no longer has to ask users to invoke socket in the endpoint. The same for Oban and pretty much every LiveView lib out there. This will be a huge improvement thanks to your work! Since there is some API work necessary to make this happen, that's better in the hands of @chrismccord! So I would expect this to happen only after v1.7 is out. :) |
|
AMAZING! I see two streams of followup work coming from this: First, I'll get to work on implementing generic upgrade / handler support via Sock as we've talked about above. I suspect that should be up for review within a week or so; I'll be dropping a few points for early discussion down below to make sure we're aligned on this. There shouldn't be any API facing work here; this is all plumbing. Do you see any reason that this couldn't make it into 1.7 assuming I get the work done soon? Second, in terms of moving socket down into router, that's going to simplify things a ton, both within Phoenix and for users as you say. Given that this will change the user facing socket definition API, I agree that this should be tackled post-1.7. |
I can give feedback once the PR is up. I am worried about the second item affecting the API from the first item but maybe that won't be the case depending on how isolated it is. |
|
I don't think there's going to be any real overlap between the two items, but I agree, let's wait and see how the PR shakes out. My main uncertainty as this point is whether the Sock API should subsume the The way it is now in In Phoenix's case, if the responsibility for getting info out of the originating This could always be accommodated later on by adding an optional step of this nature to the Sock lifecycle & moving some stuff in Any strong feelings one way or the other? |
|
Also, thanks again for being so amazing to work with. You really do make it look easy! |
Right now, I am thinking that the Sock contract would exactly be the I also really think we should have "Web" in the name of the project if it is going to be WebSocket specific. :) WebSocket, WebSock, WS are all fine by me. :) |
Agree that that's a fine place to start. My concern is really more of 'future optimization' than anything fundamentally structural.
Instead of 'Sock', you mean? The good names all mostly already taken in Hex, sadly. Though |
|
Getting this working on Bandit was a breeze! It's official: Bandit 0.5.7 (just released) will FULLY support Phoenix as of 1.7 (even if Sock support doesn't land)! Fantastic! Thanks again for all your hard work on this @josevalim! |
The work here is the first step towards implementation of the Phoenix work outlined in #5003. It implements:
Plug.Conn.upgrade_adapter/3Phoenix.Endpoint.Cowboy2Handleras they are no longer requiredNote that this PR is not entirely complete yet; it will need to have its dependencies on Plug & Plug.Cowboy updated to releases which incorporate elixir-plug/plug#1119 and elixir-plug/plug_cowboy#88 respectively. CI obviously won't pass until this is done.