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

Switched to websocket/driver and improved websocket handling #110

Closed
wants to merge 1 commit into from
Closed

Switched to websocket/driver and improved websocket handling #110

wants to merge 1 commit into from

Conversation

robertjpayne
Copy link
Contributor

This pull request should be considered a WIP, I just wanted to open it for some feedback and sharing.

In general I found WebSocket support in Reel to be a tad flaky and hard to get working since there are not many good examples of keeping a long running persistent WebSocket open in Reel.

This pull request does the following:

  • Complete re-write of the websocket handler
  • Uses websocket/driver instead of websocket_parser
  • Detaches the connection instead of just hijacking the socket
  • Easy support for long running persistent connection

What I would still like to achieve with websocket support in reel:

  • Automatic pinging of client from server if connection is idle for 30s or more
  • A way to signal and interrupt the websocket outside of client disconnect/closing
  • Specs ( I'm horrible at writing specs )
  • Examples ( Lots of examples on how to use this properly and easily! )

@robertjpayne
Copy link
Contributor Author

For anyone wanting to potentially use this without integrating the pull request I have a standalone handler here:

https://gist.github.com/robertjpayne/6961149

Unfortunately this will not integrate with webmachine nicely.

@tarcieri
Copy link
Member

Nice! Thanks for doing this! This is something we have planned on for awhile but never had time to do.

It looks like all the tests are failing though?

NameError:
       uninitialized constant WebSocket::ClientHandshake

@robertjpayne
Copy link
Contributor Author

I'll have a look at the tests, this pull request is very much a work in progress too. The actor that captures the WebSocket is pretty much locked onto the socket so I'm not entirely sure how useful it will be without some mechanism to feed messages back to the client or if that is something we should let others solve!

I'd love to port FAYE to use Celluloid/Reel instead of EventMachine but that may take a bit more effort than I have time for right now too.

@tarcieri
Copy link
Member

@robertjpayne having connections locked to a specific actor is intended behavior. It should be possible to send messages when callbacks fire within that specific actor, as well as set timers, although timers are presently broken with Celluloid::IO (see celluloid/celluloid-io#56)

@robertjpayne
Copy link
Contributor Author

Just for anyone that may be watching this I'm struggling to get a spec setup suitable for testing the websocket driver as it blocks the calling thread.

@tarcieri I'm not sure if you could help out here, maybe we can hash it out over IRC but I'm not having any luck bending rspec to my will to avoid things blowing up like mad and getting proper testing out of the specs.

@Asmod4n
Copy link
Contributor

Asmod4n commented Jan 27, 2014

blocking the calling thread means Reel can only handle one connection?
Solved: https://gist.github.com/Asmod4n/8654523 :)

@tarcieri
Copy link
Member

@Asmod4n that makes a separate thread per Websocket connection. Ideally they could all use the same thread.

@robertjpayne I'd be happy to help pair with you over IRC to get this finished. It'd be nice for Reel to have solid Websocket support

@Sen
Copy link

Sen commented Apr 26, 2014

hi @robertjpayne @tarcieri , any progress about this feature?

@tarcieri
Copy link
Member

Nope :|

@env ||= begin
e = {
:method => @request.method,
:input => @request.body.to_s,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I belive you would want to use @request.body here and not the string representation.

@Asmod4n
Copy link
Contributor

Asmod4n commented Jul 2, 2014

Have made a small gist on how to use the websocket-driver lib with celluloid-io: https://gist.github.com/Asmod4n/241c84ef31df921b31e9

@tarcieri @robertjpayne

@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

4 participants