search

celluloid / reel

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

storing WebSocket instance into @websocket instance variable, a try to fix #60 #66

Closed ghost closed 11 years ago

ghost commented 11 years ago

Not sure why @socket should be nil here... https://github.com/celluloid/reel/blob/master/lib/reel/connection.rb#L60

It is there since v0.2.0.pre (read before my updates :)) https://github.com/celluloid/reel/blob/v0.2.0.pre/lib/reel/connection.rb#L62

No time to investigate why it should be nil, but setting it to actual WebSocket instance result into Stream Closed exception.

As a workaround we can store WebSocket instance into @websocket instance variable and use it instad of @socket on websocket requests?

This is fixing #60

Makes sense?

tarcieri commented 11 years ago

Well, I can explain a bit why it works that way. The idea is a bit similar to Rack hijack: once upgraded to a WebSocket, the WebSocket is in control of the socket, not the connection. The connection should not be used after that, and setting socket to nil was flagging that fact.

I agree the semantics could be better here. I'm not sure I like your fix. I think the WebSocket should be the only object you're interacting with after the connection has been upgraded.