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
Add support for protocol upgrades #1119
Conversation
* Update a number of existing tests on :sent that were too narrow in scope * Update docs to reflect widening of scope
@josevalim as requested, this work is done and ready for review! See phoenixframework/phoenix#5003 for an overview & merge proposal. |
If I may make the gentlest & most humble request to have this work reviewed it would be greatly appreciated. I've got a pretty significant backlog of work on Bandit whose release is blocked on this. No need to get to the rest of phoenixframework/phoenix#5003, but this one PR making its way into a Plug release would really unblock forward progress on Bandit. Of course, if there's anything I can do to help, please let me know. A most gracious thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of a generic upgrade function but if we only need to support websockets for now maybe we can make it an upgrade_websocket
function instead so that we can actually document the functionality in plug itself and make it generic for all adapters?
If we need to support more upgrade protocols in the future it would be easier to reason about a generic API for it since we will have more than a single protocol to design around.
José and I had a back and forth on this exact detail previously; see the discussion from phoenixframework/phoenix#4973 (comment) downwards. The outcome of this drives a relevant response to all the other issues you've raised above. To be clear, I don't care too much one way or the other (they're both internally consistent perspectives), though I do have a minor bias towards treating this as a lower level adapter concern since it purposely keeps the upgrade semantics vague. In terms of other future uses, I'm planning on adding explicit h2c upgrade support to Bandit as part of the 0.7 release later in 2022, and will be using this same mechanism to accomplish it. Since you and José have expressed opposing (and both valid!) points on the shape of this API, I think we should let him weigh in here to get consensus. Thanks for the review! |
If you already came to a decision on this then let's go with that. I didn't realize this had already been discussed. |
@ericmj sounds good! I'll comment on your issues above with that understanding. |
This looks great to me! My only question is about We could make it
Which maybe is better than:
|
Another concern with the current API is that someone could do:
instead of:
And it would work as a no-op and can be hard to debug. Maybe we should hardcode the existing upgrades to avoid those situations? I assume it is a small list anyway? |
To your original point José, I think the intentional vagueness of |
Good catch. I think this should be the duty of the underlying sever to raise loudly on an upgrade request that isn't supported (raising is appropriate here I think, since there's no otherwise correct way to fallback in this case; it's an obvious programmer error). I can update the Plug.Cowboy and Bandit PRs to match this if we agree |
(Note that I updated opts to args above) |
When would we return |
That's a good question! I might suggest that we keep the adapter contract as 'return an ok or error tuple' and have Plug.Conn be the one to raise that as an error (ie: raise at https://github.com/elixir-plug/plug/pull/1119/files#diff-a6b7dff2ccb7fd0cb9c7d1893c4c05f3ec3c714ad04495ef87240449ed3446f3R1401). This keeps adapters from worrying about use particulars of how to surface the error, keeping that logic in a consolidated place within Plug.Conn. To be clear, this error is not 'the application requested a valid upgrade type but was unable to do the upgrade based on the client request', but rather 'the application requested an upgrade to a protocol I don't know about'. Presumably this is going to be squarely a programmer error and not a data or client error, so a raise would be appropriate. WDYT? |
Agreed on matching on |
Super. I'll have this done within the hour. |
Changes complete! To summarize the plan to get phoenixframework/phoenix#5030 ready for review into 1.7:
|
Yup, perfect! |
Co-authored-by: José Valim <jose.valim@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! I think this is good to go, let's just wait to be double sure we can tackle everything on Cowboy side. :)
lib/plug/adapters/test/conn.ex
Outdated
@@ -119,6 +119,16 @@ defmodule Plug.Adapters.Test.Conn do | |||
:ok | |||
end | |||
|
|||
def upgrade(%{owner: owner, ref: ref} = state, :supported = protocol, opts) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be :websocket
instead of :supported
? Because I think folks may want to test that a given connection upgrades at certain moments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, maybe we should allow everything and only support :unsupported
to test the :unsupported
cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd intentionally used :supported
and :unsupported
to help illustrate that this implementation was only useful to test whether or not an upgrade is supported or not (specifically, users wouldn't get confused seeing :websocket
and wondering why a full-fledged client upgrade didn't work).
The test process still gets send a message in this case, which should be sufficient for users to test against. Really, all this needs to do is be able to replicate the return value for a supported and unsupported upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as-is it means I wouldn’t be able to test that a specific route is triggering a websocket upgrade, right? Because we would get a function clause error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I see your concern now. I suppose given that this is a test plug all bets are off with respect to upgrades (or anything else, really) actually doing anything.
I'll update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lib/plug/adapters/test/conn.ex
Outdated
@@ -119,6 +119,16 @@ defmodule Plug.Adapters.Test.Conn do | |||
:ok | |||
end | |||
|
|||
def upgrade(%{owner: owner, ref: ref}, :unsupported = protocol, opts) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call it :not_supported
for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great to me! ❤️ Let's get the Cowboy one ready and I can ship both!
💚 💙 💜 💛 ❤️ |
🥳 |
Thank you @mtrudel! |
This PR is the first step on the road to overhauling how WebSocket server support is exposed to & used by Plug-based applications such as Phoenix (see phoenixframework/phoenix#5003 for background).
Support is added here for a new
Plug.Conn.upgrade_adapter/3
function, and a backingPlug.Conn.Adapter.upgrade/3
callback. The intent of this PR is solely to provide the minimal plumbing necessary for Plug applications to be able to request a protocol upgrade from the underlying web server; beyond this minimum plumbing no requirements are put on the upgrade process, supported protocols, or how a Plug application is expected to determine if a connection is suitable for upgrading.This PR represents the total extent of changes expected to the Plug API as a result of the work laid out in the linked issue above, and can be reviewed & merged as-is, or kept open & considered as part of the overall workup; it's up to you.
Companion PRs for this are at elixir-plug/plug_cowboy#88 and mtrudel/bandit#38.