search

elixir-plug / plug

Compose web applications with functions
https://hex.pm/packages/plug
Other
2.84k stars 582 forks source link

Add support for protocol upgrades #1119

Closed mtrudel closed 1 year ago

mtrudel commented 1 year ago

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 https://github.com/phoenixframework/phoenix/issues/5003 for background).

Support is added here for a new Plug.Conn.upgrade_adapter/3 function, and a backing Plug.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 https://github.com/elixir-plug/plug_cowboy/pull/88 and https://github.com/mtrudel/bandit/pull/38.

mtrudel commented 1 year ago

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

mtrudel commented 1 year ago

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!

mtrudel commented 1 year ago

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 https://github.com/phoenixframework/phoenix/pull/4973#issuecomment-1261124784 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!

ericmj commented 1 year ago

If you already came to a decision on this then let's go with that. I didn't realize this had already been discussed.

mtrudel commented 1 year ago

@ericmj sounds good! I'll comment on your issues above with that understanding.

josevalim commented 1 year ago

This looks great to me! My only question is about opts.

We could make it term, as I originally proposed, but we could also force it to be a keyword list? For example, Cowboy would do this:

upgrade_adapter(conn, :websocket, handler: {Foo, arg}, fullsweep_after: 400)

Which maybe is better than:

upgrade_adapter(conn, :websocket, {Foo, arg, fullsweep_after: 400})

Do you have any preferences @mtrudel / @ericmj?

josevalim commented 1 year ago

Another concern with the current API is that someone could do:

upgrade_adapter(conn, :websocket, opts)

instead of:

upgrade_adapter(conn, :websockets, opts)

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?

mtrudel commented 1 year ago

To your original point José, I think the intentional vagueness of term is the right way to go here. No strong feeling on it though.

mtrudel commented 1 year ago

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?

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

mtrudel commented 1 year ago

(Note that I updated opts to args above)

josevalim commented 1 year ago

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).

When would we return {:error, _} then?

mtrudel commented 1 year ago

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?

josevalim commented 1 year ago

Agreed on matching on {:error, _} and raising then!

mtrudel commented 1 year ago

Super. I'll have this done within the hour.

mtrudel commented 1 year ago

Changes complete! To summarize the plan to get phoenixframework/phoenix#5030 ready for review into 1.7:

josevalim commented 1 year ago

Yup, perfect!

josevalim commented 1 year ago

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

mtrudel commented 1 year ago

🥳

ericmj commented 1 year ago

Thank you @mtrudel!