WebSocket exceptions when trying to use BinaryJS streams #32

Closed
sorear opened this Issue Feb 1, 2013 · 5 comments

Projects

None yet

2 participants

Contributor
sorear commented Feb 1, 2013

Repeat visitor here, and apologies for what must be a very unclear question/report/thing.

If you try to do almost anything with a BinaryJS stream (on the node.js server side) after the WebSocket connection is closed, the operation will throw an exception from within streamws because readyState is not OPEN. It's not clear how this can be avoided in my code, except by checking $binaryclient._socket.readyState manually, or by wrapping all operations in try/catch. In particular:

  1. Piping data to a stream with "end" set to true will cause end to be called even if writable is false, which will result in an exception if the underlying websocket is closed.
  2. After I'm done using a stream, I want to close it to free resources. But it is not clear if there is any way to identify streams that can be safely closed.
  3. It is possible under some conditions for streamws to mark a connection closed without promptly sending a 'close' event. For instance, $binaryClient._socket.close(); if ($stream.writable) $stream.write('foo') will throw, as writable is still true after the close even though the socket's readyState is not OPEN. I don't know if this can be triggered remotely, but I am very nervous about it from a security POV.
Owner
ericz commented Apr 4, 2013

Taking a look at all these issues now

Owner
ericz commented Apr 4, 2013

Ok I added a check that the socket is open before doing any writes on streams, returning false if socket is closed (which should cause a stream to stop) this should address 1. and 2.
See: d51b2de

Owner
ericz commented Apr 4, 2013

Ok this also partially addresses 3. because now when you write it won't throw but the .write will return false.

The close event indeed doesn't fire immediately but it does soon thereafter.

Let me know if this is acceptable solution. Feel free to reopen

@ericz ericz closed this Apr 4, 2013
Owner
ericz commented Apr 4, 2013

Will be pushed in 0.1.8

Owner
ericz commented Apr 4, 2013

Pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment