search

celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
596 stars 87 forks source link

Switch to websocket-driver gem #154

Closed mwean closed 9 years ago

mwean commented 10 years ago

This is inspired by #110. I'm not sure that I'm doing everything in the correct Celluloid way, but I'm able to use it with reel-rack, so I think it's a good start. I welcome any comments/changes.

tarcieri commented 10 years ago

This sounds awesome! I hope I can review it soon.

kenichi commented 9 years ago

I can't get this to work: the tests pass but websocket clients lock it up. I have an example here that works if you change the Gemfile to pull normal reel. I tried with a rebase from master as well. I also tried putting the call to Reel::WebSocket#start_listening in a reactor task, since it seems to block there, to no avail. I'm not trying to read messages from the client yet, just send a few messages to it.

tarcieri commented 9 years ago

Unfortunately this patch has lingered for awhile. It's great you're looking at it though.

cc @digitalextremist @jcoglan

digitalextremist commented 9 years ago

Checking into this again also. I've been using both driver and parser. I worked around the locking issue recently and have that code working in production. Some time I will compare both implementations and revisit this PR.

tinco commented 9 years ago

Alright, so the news is not so good. As the fact that the specs were changed indicates, this PR breaks the external api. Instead of wrapping the Ruby driver in a way that makes it compatible with the Websocket API there was before it simply exposes it.

The reason the new code works in the specs but not in @kenichi's code is that in the specs the WebSocket is ran in a new thread each time. To make the real example work its worker has to be extracted into a proper Actor.

Another issue with the PR is that the hijacking and detaching of the socket is done in the initialize, but the listen loop is also. This makes it that you want to run the initialize in a new thread as is done in the specs, but I suspect hijacking and detaching in a new thread is I think open to a race condition.

I am afraid my recommendation would be to rewrite this PR from scratch.

tinco commented 9 years ago

I just noticed #110, in my opinion that one is cleaner and much more promising. I'll look into that one next (somewhere this week).

mwean commented 9 years ago

I tried to base my work on #110, so it should be pretty similar. I'm not very experienced with Celluloid, so it makes sense that my implementation isn't correct.

tarcieri commented 9 years ago

Fixed in #166