Client.destroy() does not always destroy the socket, node hangs on exit

[justfalter]

Client.destroy() will only destroy the underlying socket if is writable. Being able to destroy the socket is important as it frees the file descriptor, otherwise node will hang on shutdown until the socket times out (14-15 minutes, in my case).

I'm able to reproduce this by calling Client.end() just prior to calling Client.destroy(), as the first puts the socket into an unwritable state.

[mscdex]

Can you provide an example to reproduce the problem and include which node version you're using? A connecting or connected socket would always have .writable === true. Recent node versions even set .writable to true at the time the socket is created.

[justfalter]

Reproducing the problem can be a bit tough as it essentially requires the socket to end up in a half-open (FIN-WAIT-1) state.

Basically:

client.end(); // FIN sent to server
// Socket is no longer writable
client.destroy(); // nop because socket is not writable

Most SSH servers will respond to a FIN from a client with a FIN of their own, and when the client receives that FIN, nodejs calls Socket.destroy() automatically. However, if a FIN packet never arrives at the client (dropped packet or server leaves things half-open), the client socket will remain open in a FIN-WAIT-1 state. This leaves the file descriptor open in the nodejs process, and it never exits. Calling client.destroy() does nothing because the socket is not writable.

The documentation for socket.destroy() states:

Ensures that no more I/O activity happens on this socket. Destroys the stream and closes the connection.

My hope is that Client.destroy() would do the same.

[mscdex]

I was under the impression you were only calling client.destroy() in your code, not client.end() followed by client.destroy() (aside from your test in this issue).

[justfalter]

No worries, I could have been more clear.

[formula1]

https://github.com/mscdex/ssh2/blob/master/lib/client.js#L1089

Seems as though when client.destroy() is called it ensures that it's writable before destroying. I suppose you could just remove that to fix it. Is there a specific reason that was done? Should I make a pull request?