Properly handle child process exit codes

The child process 'exit' was returning the status of the process, rather than the exit code. This patch properly deconstructs the status into the exit code and the term signal a process may have received. See: http://pod.tst.eu/http://cvs.schmorp.de/libev/ev.pod#Watcher_Specific_Functions_and_Data_-5 and waitpid(2)
15 years ago · f8a3cf980f
9 changed files with 205 additions and 20 deletions
--- a/doc/api.markdown
+++ b/doc/api.markdown
@ -849,12 +849,17 @@ Child processes always have three streams associated with them. `child.stdin`,

 ### Event:  'exit'

-`function (code) {} `
+`function (code, signal) {} `

-This event is emitted after the child process ends. `code` is the final exit
-code of the process.  After this event is emitted, the `'output'` and
-`'error'` callbacks will no longer be made.
+This event is emitted after the child process ends. If the process terminated
+normally, `code` is the final exit code of the process, otherwise `null`. If
+the process terminated due to receipt of a signal, `signal` is the string name
+of the signal, otherwise `null`.

+After this event is emitted, the `'output'` and `'error'` callbacks will no
+longer be made.
+
+See `waitpid(2)`

 ### child_process.spawn(command, args, env)

@ -906,8 +911,8 @@ be sent `'SIGTERM'`. See `signal(7)` for a list of available signals.
        spawn = require('child_process').spawn,
        grep  = spawn('grep', ['ssh']);

-    grep.addListener('exit', function (code) {
-      sys.puts('child process exited with code ' + code);
+    grep.addListener('exit', function (code, signal) {
+      sys.puts('child process terminated due to receipt of signal '+signal);
    });

    // send SIGHUP to process
@ -1013,7 +1018,8 @@ output, and return it all in a 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 `err.code`
-will be the exit code of the child process.
+will be the exit code of the child process, and `err.signal` will be set to the
+signal that terminated the process.

 There is a second optional argument to specify several options. The default options are

--- a/lib/child_process.js
+++ b/lib/child_process.js
@ -70,14 +70,15 @@ exports.execFile = function (file, args /*, options, callback */) {
    }
  });

-  child.addListener("exit", function (code) {
+  child.addListener("exit", function (code, signal) {
    if (timeoutId) clearTimeout(timeoutId);
-    if (code == 0) {
+    if (code === 0 && signal === null) {
      if (callback) callback(null, stdout, stderr);
    } else {
      var e = new Error("Command failed: " + stderr);
      e.killed = killed;
      e.code = code;
+      e.signal = signal;
      if (callback) callback(e, stdout, stderr);
    }
  });
@ -91,6 +92,7 @@ function ChildProcess () {

  var gotCHLD = false;
  var exitCode;
+  var termSignal;
  var internal = this._internal = new InternalChildProcess();

  var stdin  = this.stdin  = new Stream();
@ -99,16 +101,17 @@ function ChildProcess () {

  stderr.onend = stdout.onend = function () {
    if (gotCHLD && !stdout.readable && !stderr.readable) {
-      self.emit('exit', exitCode);
+      self.emit('exit', exitCode, termSignal);
    }
  };

-  internal.onexit = function (code) {
+  internal.onexit = function (code, signal) {
    gotCHLD = true;
    exitCode = code;
+    termSignal = signal;
    stdin.end();
    if (!stdout.readable && !stderr.readable) {
-      self.emit('exit', exitCode);
+      self.emit('exit', exitCode, termSignal);
    }
  };

--- a/src/node.cc
+++ b/src/node.cc
@ -602,6 +602,152 @@ static inline const char *errno_string(int errorno) {
  }
 }

+const char *signo_string(int signo) {
+#define SIGNO_CASE(e)  case e: return #e;
+  switch (signo) {
+
+#ifdef SIGHUP
+  SIGNO_CASE(SIGHUP);
+#endif
+
+#ifdef SIGINT
+  SIGNO_CASE(SIGINT);
+#endif
+
+#ifdef SIGQUIT
+  SIGNO_CASE(SIGQUIT);
+#endif
+
+#ifdef SIGILL
+  SIGNO_CASE(SIGILL);
+#endif
+
+#ifdef SIGTRAP
+  SIGNO_CASE(SIGTRAP);
+#endif
+
+#ifdef SIGABRT
+  SIGNO_CASE(SIGABRT);
+#endif
+
+#ifdef SIGIOT
+# if SIGABRT != SIGIOT
+  SIGNO_CASE(SIGIOT);
+# endif
+#endif
+
+#ifdef SIGBUS
+  SIGNO_CASE(SIGBUS);
+#endif
+
+#ifdef SIGFPE
+  SIGNO_CASE(SIGFPE);
+#endif
+
+#ifdef SIGKILL
+  SIGNO_CASE(SIGKILL);
+#endif
+
+#ifdef SIGUSR1
+  SIGNO_CASE(SIGUSR1);
+#endif
+
+#ifdef SIGSEGV
+  SIGNO_CASE(SIGSEGV);
+#endif
+
+#ifdef SIGUSR2
+  SIGNO_CASE(SIGUSR2);
+#endif
+
+#ifdef SIGPIPE
+  SIGNO_CASE(SIGPIPE);
+#endif
+
+#ifdef SIGALRM
+  SIGNO_CASE(SIGALRM);
+#endif
+
+  SIGNO_CASE(SIGTERM);
+  SIGNO_CASE(SIGCHLD);
+
+#ifdef SIGSTKFLT
+  SIGNO_CASE(SIGSTKFLT);
+#endif
+
+
+#ifdef SIGCONT
+  SIGNO_CASE(SIGCONT);
+#endif
+
+#ifdef SIGSTOP
+  SIGNO_CASE(SIGSTOP);
+#endif
+
+#ifdef SIGTSTP
+  SIGNO_CASE(SIGTSTP);
+#endif
+
+#ifdef SIGTTIN
+  SIGNO_CASE(SIGTTIN);
+#endif
+
+#ifdef SIGTTOU
+  SIGNO_CASE(SIGTTOU);
+#endif
+
+#ifdef SIGURG
+  SIGNO_CASE(SIGURG);
+#endif
+
+#ifdef SIGXCPU
+  SIGNO_CASE(SIGXCPU);
+#endif
+
+#ifdef SIGXFSZ
+  SIGNO_CASE(SIGXFSZ);
+#endif
+
+#ifdef SIGVTALRM
+  SIGNO_CASE(SIGVTALRM);
+#endif
+
+#ifdef SIGPROF
+  SIGNO_CASE(SIGPROF);
+#endif
+
+#ifdef SIGWINCH
+  SIGNO_CASE(SIGWINCH);
+#endif
+
+#ifdef SIGIO
+  SIGNO_CASE(SIGIO);
+#endif
+
+#ifdef SIGPOLL
+  SIGNO_CASE(SIGPOLL);
+#endif
+
+#ifdef SIGLOST
+  SIGNO_CASE(SIGLOST);
+#endif
+
+#ifdef SIGPWR
+  SIGNO_CASE(SIGPWR);
+#endif
+
+#ifdef SIGSYS
+  SIGNO_CASE(SIGSYS);
+#endif
+
+#ifdef SIGUNUSED
+  SIGNO_CASE(SIGUNUSED);
+#endif
+
+  default: return "";
+  }
+}
+

 Local<Value> ErrnoException(int errorno,
                            const char *syscall,
--- a/src/node.h
+++ b/src/node.h
@ -80,5 +80,6 @@ v8::Local<v8::Value> ErrnoException(int errorno,
                                    const char *syscall = NULL,
                                    const char *msg = "");

+const char *signo_string(int errorno);
 }  // namespace node
 #endif  // SRC_NODE_H_
--- a/src/node_child_process.cc
+++ b/src/node_child_process.cc
@ -1,5 +1,6 @@
 // Copyright 2009 Ryan Dahl <ry@tinyclouds.org>
 #include <node_child_process.h>
+#include <node.h>

 #include <assert.h>
 #include <string.h>
@ -244,7 +245,7 @@ int ChildProcess::Spawn(const char *file,
 }


-void ChildProcess::OnExit(int code) {
+void ChildProcess::OnExit(int status) {
  HandleScope scope;

  pid_ = -1;
@ -258,10 +259,20 @@ void ChildProcess::OnExit(int code) {

  TryCatch try_catch;

-  Local<Value> argv[1];
-  argv[0] = Integer::New(code);
+  Local<Value> argv[2];
+  if (WIFEXITED(status)) {
+    argv[0] = Integer::New(WEXITSTATUS(status));
+  } else {
+    argv[0] = Local<Value>::New(Null());
+  }
+
+  if (WIFSIGNALED(status)) {
+    argv[1] = String::NewSymbol(signo_string(WTERMSIG(status)));
+  } else {
+    argv[1] = Local<Value>::New(Null());
+  }

-  onexit->Call(handle_, 1, argv);
+  onexit->Call(handle_, 2, argv);

  if (try_catch.HasCaught()) {
    FatalException(try_catch);
--- a/test/fixtures/exit.js
+++ b/test/fixtures/exit.js
@ -0,0 +1 @@
+process.exit(process.argv[2] || 1);
--- a/test/simple/test-child-process-exit-code.js
+++ b/test/simple/test-child-process-exit-code.js
@ -0,0 +1,11 @@
+require("../common");
+var spawn = require('child_process').spawn
+  , path = require('path')
+  , sub = path.join(fixturesDir, 'exit.js')
+  , child = spawn(process.argv[0], [sub, 23])
+  ; 
+
+child.addListener('exit', function(code, signal) {
+  assert.strictEqual(code, 23);
+  assert.strictEqual(signal, null);
+});
--- a/test/simple/test-child-process-kill.js
+++ b/test/simple/test-child-process-kill.js
@ -2,7 +2,8 @@ require("../common");

 var spawn = require('child_process').spawn;

-var exitStatus = -1;
+var exitCode;
+var termSignal;
 var gotStdoutEOF = false;
 var gotStderrEOF = false;

@ -25,14 +26,16 @@ cat.stderr.addListener("end", function () {
  gotStderrEOF = true;
 });

-cat.addListener("exit", function (status) {
-  exitStatus = status;
+cat.addListener("exit", function (code, signal) {
+  exitCode = code;
+  termSignal = signal;
 });

 cat.kill();

 process.addListener("exit", function () {
-  assert.ok(exitStatus > 0);
+  assert.strictEqual(exitCode, null);
+  assert.strictEqual(termSignal, 'SIGTERM');
  assert.ok(gotStdoutEOF);
  assert.ok(gotStderrEOF);
 });
--- a/test/simple/test-exec.js
+++ b/test/simple/test-exec.js
@ -23,6 +23,7 @@ exec("ls /DOES_NOT_EXIST", function (err, stdout, stderr) {
    assert.equal("", stdout);
    assert.equal(true, err.code != 0);
    assert.equal(false, err.killed);
+    assert.strictEqual(null, err.signal);
    puts("error code: " + err.code);
    puts("stdout: " + JSON.stringify(stdout));
    puts("stderr: " + JSON.stringify(stderr));
@ -36,11 +37,13 @@ exec("ls /DOES_NOT_EXIST", function (err, stdout, stderr) {
 exec("sleep 10", { timeout: 50 }, function (err, stdout, stderr) {
  assert.ok(err);
  assert.ok(err.killed);
+  assert.equal(err.signal, 'SIGKILL');
 });

 exec('python -c "print 200000*\'C\'"', { maxBuffer: 1000 }, function (err, stdout, stderr) {
  assert.ok(err);
  assert.ok(err.killed);
+  assert.equal(err.signal, 'SIGKILL');
 });

 process.addListener("exit", function () {