Switch to websocket driver

[tinco]

This is the third effort to switch to the websocket driver ;) I'm basing my PR off @robertjpayne's #110.

This PR is not ready yet, I'm just creating it so it's a little bit more visible that I'm working on it.

I expect to be done within a few days. My goal is to not break the original API.

Small question for @tarcieri: I really like the Actor style code where you just call 'read' and it will suspend the fiber and resume when there's a result, but beside that interface you also implemented a read_every method with on_* handlers in the event style, why is that? Was it just convenient as websocket_parser already emitted those events or would you encourage people to use the class in that way?

In any case I'll be supporting both use cases, as to not break the original API :)

[tinco]

Had a productive evening, the example works now. Unfortunately the example does not try to read. I'll run the test suite tomorrow and see if there's some stuff to clean up.

@tarcieri would you like me to squash the commits before merging?

[tarcieri]

No, keeping the history seems fine. I'm gone this weekend, but perhaps @digitalextremist can provide some initial review.

[digitalextremist]

Will do when I get a chance. Been watching this closely.

On February 5, 2015 4:47:52 PM PST, Tony Arcieri notifications@github.com wrote:

No, keeping the history seems fine. I'm gone this weekend, but perhaps
@digitalextremist can provide some initial review.

---

Reply to this email directly or view it on GitHub:
#166 (comment)

[tinco]

So the latest push makes the specs pass. I still want to do some additional checks to make sure my code is robust and I still have to move back in the evented code.

@tarcieri I also stumbled into something that might be a small security bug in the old websocket code. Apparently it accepted unmasked messages, which has some kind of security consequence explained here:

http://stackoverflow.com/questions/14174184/what-is-the-mask-in-a-websocket-frame

I noticed because the ruby-websocket-driver does not accept unmasked messages, and in the specs it sent unmasked messages for testing.

[tinco]

Nvm the security thing, I just got what it's about, and the whole thing is super theoretical. The idea is that having Javascript clients send random byte streams over sockets is dangerous, so the streams are XOR'ed with random numbers by the browser. What kind of proxies would be vulnerable and why those proxies wouldn't be targeted by non-browser websocket implementations is beyond me :P

[tinco]

I made some specs for the event style methods. They pass in master, and fail in this branch.

[tinco]

Alright, I think I'm done for now. Let me know what you think :) I'll continue on my little project now.

[digitalextremist]

@d-snp will be back to review this over the next few days.

[tarcieri]

Same here, I just got back from vacation and will try to look at this a bit later.

[Asmod4n]

There is also a way to use Websocket-Driver without that rack stuff: https://gist.github.com/Asmod4n/241c84ef31df921b31e9/0f7d3d3d04ffdf508ebb301ad141cddfd3830f7b

[tinco]

Ah that's excellent, I'll fix that then :)

[tinco]

So the way reel works right now is that reel parses the headers before it creates the driver. The code @Asmod4n proposed instead lets the driver parse it first. Internally the driver parses the headers into a rack-like datastructure and then calls Driver.rack anyway. I don't think we want Reel to use ruby-websocket-driver's http parser for everything so I still think the PR as it is stands :)

[tarcieri]

@d-snp the reason it did that was so it could spot the Upgrade header and switch into Websocket mode. Since this only happens on the initial handshake I'm not sure what the problem is. I do remember encountering this when trying to do this conversion initially.

If you've avoided it, great! Using ruby-websocket-driver for every request seems suboptimal, since we'd lose the performance benefits of the native parsing http_parser.rb affords.

[tinco]

There's no problem, it's just that WebSocket::Driver.server will try to parse the headers again, so to make it work we would have to reserialize the headers and let it parse those, just so that it will internally turn those into the Rack format and then do what we're already doing now.

[tarcieri]

I'd rather pay a small penalty to set up WebSocket connections than slow down normal HTTP requests in the name of WebSocket support

[digitalextremist]

I agree on avoiding altering the server to accommodate WebSocket requests at any cost at all to ordinary protocol requests. That has been a huge priority to me in handling WebSockets.

Sorry I've not been back to review yet, still sorting out some situations as quickly as I can.

On February 11, 2015 3:29:09 PM PST, Tony Arcieri notifications@github.com wrote:

I'd rather pay a small penalty to set up WebSocket connections than
slow down normal HTTP requests in the name of WebSocket support

---

Reply to this email directly or view it on GitHub:
#166 (comment)

[ioquatix]

I am going to review this code.

[ioquatix]

This PR seems fine. But, I have a problem with it. Is there any reason why WebSocket support needs to be directly and specifically coded into reel? Because, technically, as long as the request is passed along correctly, the client code could handle it as required using an API similar to the rack.hijack API. In fact, handling it at this level might prevent a correct implementation of rack.hijack API (or whatever that ends up being in Rack 2.0).

Implementing a celluloid websocket client/server is all of 20-30 lines of code, and can easily fit on top of reel. This keeps reel small and focused, and allows clients to upgrade to websockets if and when appropriate.

[ioquatix]

There is one other issue to consider here. The main server is using Celluloid::IO for handling requests concurrently, but the request is passed off to a callback. Is that callback still running within the reactor or is it running on the main thread?

The concern I'd have is handling long running websockets - are these running on the same IO reactor?

[tinco]

@ioquatix regarding your first issue: I have made a gem out of it called celluloid-websocket, I just made this pull request to contribute back to reel, as it already had a websocket implementation. Whether that should be removed is not related to this PR I think and should be a separate issue.

second issue: I think someone who knows more about celluloid::io has to answer that. I've not written anything specific to celluloid, it just does readpartial/write on the socket given to it by reel. If I understand correctly the way this would work is that listens for the on_open and then operates it from its own thread.

[tarcieri]

This looks good to me

[mighe]

After this PR, Angelo log crashes because WebSocket does not expose anymore peeraddr.
Is this a mistake or should we update Angelo to conform to that new API?

[tarcieri]

@mighe no, we should redelegate these methods in Reel

[tinco]

@mighe, @tarcieri sorry my bad, I forgot to expose those. I'll file another PR to fix those.

Btw @tarcieri you can close #110 and #154 since you merged this one.