Plugin hooks for WebSockets #1849
Conversation
|
How would this impact #1809? |
|
They re mutually exclusive. If Evan thinks this is better we can close the
other.
…On Tuesday, 16 July 2019, Nate Berkopec ***@***.***> wrote:
How would this impact #1809 <#1809>?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1849?email_source=notifications&email_token=AAAFT56GWO7WF6IQ5G7676TP7ZJLXA5CNFSM4IEIU3XKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2CRG4A#issuecomment-512037744>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAFT53HDGEQ5UWWM72KCB3P7ZJLXANCNFSM4IEIU3XA>
.
--
Jesús Burgos Maciá
|
|
Hi @evanphx, if we wanted to move forward having server-level support for WebSockets, would you prefer the approach from #1809 or something like what's in this PR? I understand reviewing this PR is quite demanding but, even without reading it thoroughly, it would help me a ton if I can just discard one of the two approaches. Thanks for your time! |
lib/puma/server.rb
Outdated
| @@ -652,6 +672,10 @@ def handle_request(req, lines) | |||
| # | |||
| after_reply = env[RACK_AFTER_REPLY] = [] | |||
|
|
|||
| unless @on_before_rack.nil? | |||
There was a problem hiding this comment.
This should just be if @on_before_rack.
|
Hi @Jesus. I'd like to help you get these webhook PRs over the line. Can you email me at the address on my GH profile? |
|
@Jesus and I spoke about this offline. We agreed that I would come in and provide some feedback here on the direction, and then close this PR. Jesus said he doesn't have quite as much time to work on this as he used to. I think if someone wanted to take up the issue of websocket support in Puma, taking over this PR would be a good place to start. |
| ## Per request hooks | ||
|
|
||
| `#on_before_rack(env)` will be called right before the Rack application is | ||
| invoked. The called hook may modify `env` just like any Rack middleware. |
There was a problem hiding this comment.
The way you've phrased this: "may modify X just like any Rack middleware" makes me wonder if we should just expand this to a "puma middleware" concept, where puma middleware are otherwise like Rack middleware but may also return an object that responds to stream? rather than a rack response. That would allow you to "stack" multiple on/before hooks, in order.
There was a problem hiding this comment.
That's an interesting idea, I'd like to think more about its implications but right now I believe it makes sense.
| to `#stream?` with a truthy value. Check out `lib/puma/stream_client.rb` to | ||
| know more about this interface. | ||
|
|
||
| If more than one plugin arises interest in taking over, an exception will |
There was a problem hiding this comment.
The proposed "puma middleware" idea would allow us to handle this case more gracefully.
|
|
||
| if stream_client && stream_client.stream? | ||
| if is_async | ||
| raise "Only one #on_after_rack hook should take over" |
There was a problem hiding this comment.
I think we can avoid this by just giving the user some notion of an "insertion order" - insert this plugin at this point the stack, then this plugin, etc, and the first plugin that is_async gets to take over.
| @@ -0,0 +1,72 @@ | |||
| module Puma | |||
There was a problem hiding this comment.
I think this part -> onward would be in a plugin gem, not in Puma core.
|
LGTM @Jesus. I think the interface is solid and easy to understand. A few comments about the concept of before/after handlers, but otherwise I like the direction a lot! |
|
Thanks for the feedback Nate, hopefully I can resurrect this soon. (Or maybe someone else will...) |
This is a proof of concept and it's not ready for merge at all, I'm opening this PR just to gauge interest.
What I'm trying here is to design a minimal interface that would allow an external plugin to implement WebSocket support. This has some advantages for Puma compared to the previous proposal:
websocket-driverwon't be a dependency.But also some minor disadvantages:
The current prototype as it stands has been able to run a basic chat application and this ActionCable experiment, using this
puma-websocketsplugin.I understand this PR as it is doesn't cover many edge cases and it lacks tests, but I'd be happy to work further if you think this approach is worth more time.