Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

Switch to websocket-driver gem #154

Closed
wants to merge 5 commits into from
Closed

Conversation

mwean
Copy link

@mwean mwean commented Jul 11, 2014

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
Copy link
Member

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

@kenichi
Copy link
Member

kenichi commented Jan 20, 2015

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
Copy link
Member

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

cc @digitalextremist @jcoglan

@digitalextremist
Copy link
Member

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
Copy link
Contributor

tinco commented Feb 2, 2015

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
Copy link
Contributor

tinco commented Feb 2, 2015

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
Copy link
Author

mwean commented Feb 2, 2015

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.

@tinco tinco mentioned this pull request Feb 23, 2015
@tarcieri
Copy link
Member

Fixed in #166

@tarcieri tarcieri closed this Feb 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants