spmjs / node-scp2

[MAINTAINER WANTED] A pure javascript scp program based on ssh2.
384 stars 96 forks source link

Listening to 'error' event is required when having bad ssh authentication credentials. #67

Open Brmm opened 8 years ago

Brmm commented 8 years ago

If client is not listening to the error event when authenticating with bad credentials an uncaughtException will be thrown which is hard to try/catch and will crash your app instead of giving the error in the callback.

var scp = require('scp2');
var client = new scp.Client({
    port: 22,
    host: 'localhost',
    username: 'test',
    password: 'pass'
});

//client.on('error', function(err) {
//  console.log('got event error', err)
//})

client.upload('/tmp/wat.tmp', '/srv/www/wat.tmp', function(err) {
    console.log('got err', err)
});

Uncommenting the listener in the code above will cause the callback to pass the err however this should be fixed or better documented in the README.

I handle my err logic in the callback so the fix for me is to add a client.on('error', function(){}) somewhere

datGryphon commented 5 years ago

I agree. This should be replaced/better documented. The issue is that in addition to employing the callback style async, the Client class also inherits from event emitter. This leads to a situation inside of Client.prototype.sftp. It creates it's own event listener to catch errors coming from the underlying ssh2 module. When an error is encountered, the Client uses both styles of error handling.

ssh.on('error', function(err) { self.emit('error', err); callback(err); });

I believe your solution works around the problem because it bypasses the EventEmitter's default functionality to throw the error when no listener is attached.

The real solution would be to rewrite the code to only adhere to one type of error handling pattern or the other. (Probably the event based way because there are many other places throughout the module where it is appropriate to attach event listeners.) Then make is painfully clear in the documentation that all errors are handled by events and not by callbacks.

Otherwise, with more work you could probably convert everything to promises.