driver.close() before driver.start() should close

[glasser]

If you call close on the driver before calling start, it's currently ignored. You might think this is just a matter of "don't use the API that way". But when it's being run from Faye WebSocket.Client, you don't really have a choice: start is invoked automatically and asynchronously when the TCP connection succeeds, and you definitely might want to change your mind and close the socket sometime between construction and TCP success. I will create a PR for faye/faye-websocket-node as well adding a test (which depends on this PR).

[glasser]

This is tested by faye/faye-websocket-node#29.

[glasser]

Another possibility would be to keep this module the same and declare that close before start is an error. Then make a more complex change to faye-websocket-node, something like:

• Make the change I already made which transitions readyState from CONNECTING to CLOSING on api.close()
• Change the on('connect') in client.js to check readyState and if it's CLOSING instead of CONNECTING, don't call driver.start and instead call self._finalize. (There's nothing necessary to clean up the driver?)

Although I think this would allow for a place where you call close and then _stream gets an error and you get the stream error with your close event instead of the "normal close" error, which I think is wrong.

[jcoglan]

I've not reviewed these changes by eye yet but I have run them past the Autobahn test suite and there don't seem to be any regressions.

[jcoglan]

Can you explain what the effect of ignoring close() calls before start() is? What bugs does it lead to downstream and how does this fix them?

[jcoglan]

In response to your "Another possibility" comment: I tend to prefer solutions that don't require the caller to inspect the state of its collaborators. Instead, the callee should have a sensible interpretation for all possible sequences of messages (method calls) it receives, and its state should be encapsulated. The caller should just tell the driver what it wants, not ask the driver questions. See my recent tweets and my article on websocket-driver for some further explanation of this.

[glasser]

Yeah, I agree that the "another possibility" solution isn't as good as the one I implemented here, which is why I sent in this one.

The bug I'm seeing is, I have code that uses faye-websocket-node and creates one of its Client objects. The Client constructor creates a driver and a TCP connection, and sets an on-connect on the TCP connection to start the driver.

But let's say your code decides for whatever reason in some point between the constructor returning and the success of the TCP connection that it should close the Client. That will lead to calling close on the driver object before start. But without this change, close before start is a no-op. This is pretty surprising to me, since I would think that calling close on a Client object would have the semantics "close the connection", not "close the connection if the websocket handshake is done or in progress but not if the TCP handshake isn't done yet". This lead to leaks of websockets in my code, as well as confusion where open events were being received on the Client after I tried to close it.