Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
justfalter opened this issue Jan 28, 2022 · 5 comments · May be fixed by #1125
Open

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

justfalter opened this issue Jan 28, 2022 · 5 comments · May be fixed by #1125

Comments

@justfalter
Copy link

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
Copy link
Owner

mscdex commented Jan 28, 2022

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 added a commit to justfalter/ssh2 that referenced this issue Jan 28, 2022
If client.destroy() is called, always destroy the underlying socket so
that file descriptor is freed. This matches behavior of node's Socket and
Writable.

Previous behavior made this conditional on writability of the socket.
If the socket wasn't writable, then client.destroy() wouldn't actually
destroy the socket, the file descriptor would remain open, and the
node process would hang on exit.. It might timeout after 15 minutes, if
you're lucky.

fixes mscdex#1124
@justfalter justfalter linked a pull request Jan 28, 2022 that will close this issue
@justfalter
Copy link
Author

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
Copy link
Owner

mscdex commented Jan 28, 2022

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
Copy link
Author

No worries, I could have been more clear.

@formula1
Copy link

formula1 commented Feb 3, 2022

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?

formula1 added a commit to formula1/ssh2 that referenced this issue Feb 3, 2022
mscdex#1124

Seems as though the socket can still receive data. If there's a specific reason why destroy isn't called regardless of writable state I'm willing to listen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants