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.

Sign up for GitHub

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

Fix spawn error handling #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix spawn error handling #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions lib/job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,9 @@ Job.prototype = {

this.config.spawn(cmd.command, cmd.args, options, function (err, proc) {
if (err) {
if (err === 127) {
return next(127, '', 'Command not found');
}

console.log('fail!', err, cmd.command, cmd.args);
return next(500, '', 'Unable to execude command :( ' + err);
// Shell exit code 1 represents general/unknown error
var msg = err.message || err.toString() || 'Unknown Error';
return next(1, '', 'Unable to execute command: ' + msg);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not liking that we use stdout as a way to pass error messages. I feel like we should always return an error object if we have one, and next(null, exitcode, stdout, stderr) if not. A "bad" shell code and a js-level error are totally different, no? That would change a semi-public API though.

Or maybe just use the same err.code scheme as node's child_process.exec: https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback

The callback gets the arguments (error, stdout, stderr). On success, error will be null. On error, error will be an instance of Error and error.code will be the exit code of the child process, and error.signal will be set to the signal that terminated the process.

}

proc.stdout.setEncoding('utf8');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

500 is an invalid shell exit code, which are always < 256.

Expand All @@ -218,7 +215,12 @@ Job.prototype = {
std.err += buf;
});

proc.on('exit', function(exitCode) {
var finished = false;
var done = function (exitCode) {
// Guard against double calls.
if (finished) return;
finished = true;

var end = new Date();
var elapsed = end.getTime() - start.getTime();

Expand All @@ -245,10 +247,15 @@ Job.prototype = {
}

next(exitCode, std.out, std.err);
});
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://nodejs.org/api/child_process.html#child_process_event_error

Note that the exit-event may or may not fire after an error has occurred. If you are listening on both events to fire a function, remember to guard against calling your function twice.

So we do need an error handler.

proc.on('exit', done);

proc.on('error', function() {
// prevent node from throwing an error
proc.on('error', function(err) {
// Add err message to STDOUT
std.err = std.err + '\n' + (err.message || err.toString() || 'Unknown Error');
// Shell exit code 1 represents general/unknown error
done(1);
});

var strCmd = shellQuote([cmd.command].concat(cmd.args));
Expand Down
9 changes: 7 additions & 2 deletions lib/spawn-normal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@

var spawn = require('child_process').spawn
var _ = require('lodash')

module.exports = function (command, args, options, done) {
// Prevent https://github.com/joyent/node/issues/9158
options = _.extend({}, options)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A shell exit code isn't expected here, since no command has been run. Should just pass the error as usual.

// Trap any other synchronous errors
try {
done(null, spawn(command, args, options))
} catch (e) {
done(127)
} catch (err) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spawn shouldn't ever throw unless there's a bug in node, which of course there is: nodejs/node-v0.x-archive#9158

done(err)
}
}