steelbrain / node-ssh

SSH2 with Promises
MIT License
947 stars 94 forks source link

Correctly handle unexpected errors #421

Open n1xx1 opened 2 years ago

n1xx1 commented 2 years ago

I'm using openssh with windows and it looks like it doesn't behave like node-ssh/ssh2 expects and an unhandled exception is thrown.

events.js:292
      throw er; // Unhandled 'error' event
      ^
Error: read ECONNRESET
    at TCP.onStreamRead (internal/stream_base_commons.js:209:20)
Emitted 'error' event on Client instance at:
    at Socket.<anonymous> (/usr/local/lib/node_modules/n8n/node_modules/ssh2/lib/client.js:745:12)
    at Socket.emit (events.js:315:20)
    at emitErrorNT (internal/streams/destroy.js:106:8)
    at emitErrorCloseNT (internal/streams/destroy.js:74:3)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  errno: -104,
  code: 'ECONNRESET',
  syscall: 'read',
  level: 'client-socket'
}

It looks like there isn't a generic "onerror" handler outside of the async functions, so node complains about an unhandled throw. There should be a way to always handle the error event even if outside of a single async function call, so that a faulty open-ssh implementation like the windows one doesn't crash the node process.

ghost commented 2 years ago

unrelated, but this usually happens when you do an ssh command during the connection. you should await the ssh.connect() call before running any ssh code

mikey-t commented 1 year ago

I ran into this because I sent the command sudo reboot which obviously terminates the connection, but it would be nice if I could catch the error instead of having my entire node process crash. This particular situation (running a reboot command) is relatively easy to workaround by using a different approach (or using the AWS SDK in my case), but I still think all errors should be catchable.

steelbrain commented 1 year ago

Thats a good pointer! I’ll look into this, thanks!

steelbrain commented 1 year ago

I was unable to reproduce this. For me it just gives an empty response and then closes connection

Here's the code I used ```ts import fs from 'fs' import { NodeSSH } from './index' async function main() { const ssh = new NodeSSH() await ssh.connect({ port: 22, host: '192.168.10.176', username: 'pi', password: '[redacted]', }) ssh.connection?.on('error', (err) => { console.log('error', err) }) console.log(await ssh.execCommand('echo hi')) console.log(await ssh.execCommand('sudo reboot')) } main() process.on('unhandledRejection', (reason, promise) => { console.log('dying on unhandledRejection', reason, promise) process.exit(1) }) process.on('uncaughtException', (err, origin) => { fs.writeSync(process.stderr.fd, `Caught exception: ${err}\nException origin: ${origin}`) process.exit(1) }) ```
and here's the output ``` ~/P/s/node-ssh ❯❯❯ node --trace-warnings lib/cjs/test.js { code: 0, signal: null, stdout: 'hi', stderr: '' } { code: null, signal: null, stdout: '', stderr: '' } ```

Is anybody still experiencing this?

mikey-t commented 1 year ago

Yep, I still have this issue. I spun up a project with the dependency versions from the project where I have issues:

https://github.com/mikey-t/node-ssh-reboot-test

I put notes in the readme for how to repro. I'm using typescript and ts-node with node 18.18.0. Here's what I found:

If you omit the event handler:

ssh.connection?.on('error', (err) => {
  console.log('error', err)
})

Then try/catch is ignored and it can only be caught with:

process.on('uncaughtException', (err, origin) => { }

BUT, it only happens sometimes - maybe 1 out of every 5 to 8 times for me. I'm looking at the the code that handles the promise rejection and I'm puzzled as to why this would only happen sometimes. Got any ideas?

mikey-t commented 1 year ago

I built node-ssh within WSL (ubuntu) and linked/referenced that version and was unable to reproduce. So I built node-ssh on windows and linked and also wasn't able to reproduce. So then I unlinked and pulled the live npm version down again and was able to reproduce after about 5 tries.

I'm tempted to just wire up that ssh.connection.on in my other project and walk away since that seems to prevent catch blocks from being bypassed for some reason.

thoukydides commented 3 months ago

I have just stumbled over this same issue.

Calling ssh.dispose() always results in:

Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:217:20) {
  errno: -104,
  code: 'ECONNRESET',
  syscall: 'read',
  level: 'client-socket'
}

This is with:

Adding ssh.connection?.on('error', (err) => {}); before the disconnect suppresses the error.

The SSH connection is otherwise working perfectly. This is my minimum test case that reproduces the issue:

import { NodeSSH } from 'node-ssh';
(async () => {
    const ssh = new NodeSSH();
    await ssh.connect({
        host:           SERVER_HOSTNAME,
        username:       SSH_USERNAME,
        privateKeyPath: SSH_PRIVATE_KEY_PATH
    });
    await ssh.dispose();
})();

It doesn't matter whether any execCommand is performed inbetween. Neither does adding a delay.

steelbrain commented 3 months ago

Thanks for reporting @thoukydides

.dispose method internally calls .end() on the client connection as can be seen here: https://github.com/steelbrain/node-ssh/blob/40683af8f6a676abfb32e2d821550d11dc5d75c5/src/index.ts#L930

As per ssh2's docs (https://github.com/mscdex/ssh2?tab=readme-ov-file#connection-methods) this is a valid method to call so the connection reset error here is really odd.

Maybe we should reproduce the above snippet using the underlying ssh2 APIs (https://github.com/mscdex/ssh2) and see if the error persists and if so, open an issue upstream

thoukydides commented 3 months ago

I can reproduce the same problem with ssh2 directly:

const { readFileSync } = require('fs');
const { Client } = require('ssh2');

const connection = new Client();
connection
    .on('ready', () => connection.end())
    .connect({
        host:       HOSTNAME,
        port:       22,
        username:   USERNAME,
        privateKey: readFileSync(PRIVATE_KEY_PATH)
    });

This appears to be mscdex/ssh2#850 and mscdex/ssh2#1018 (possibly a few others).

Perhaps a note in the README file would be appropriate?