From b26759aeb69cf5edbd9c23babf94c803d150d561 Mon Sep 17 00:00:00 2001 From: Niall O'Higgins Date: Mon, 29 Jul 2013 10:16:51 -0700 Subject: [PATCH 01/11] save exit status of child via SIGCHLD handler and waitpid(3) may be race-y due to always storing latest in global. might want to use a map of some sort. --- lib/pty.js | 4 ++++ src/unix/pty.cc | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/lib/pty.js b/lib/pty.js index 85a084c..7514854 100644 --- a/lib/pty.js +++ b/lib/pty.js @@ -303,6 +303,10 @@ Terminal.prototype.__defineGetter__('process', function() { return pty.process(this.fd, this.pty) || this.file; }); +Terminal.prototype.__defineGetter__('status', function() { + return pty.status(); +}); + Terminal.prototype._close = function() { this.socket.writable = false; this.socket.readable = false; diff --git a/src/unix/pty.cc b/src/unix/pty.cc index ba09422..c4e5dc5 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -22,6 +23,7 @@ #include #include #include +#include #include /* forkpty */ @@ -73,6 +75,9 @@ PtyResize(const Arguments&); static Handle PtyGetProc(const Arguments&); +static Handle +PtyGetStatus(const Arguments&); + static int pty_execvpe(const char *, char **, char **); @@ -92,9 +97,22 @@ pty_forkpty(int *, char *, const struct termios *, const struct winsize *); +static pid_t CHILD_PID; +static int CHILD_STATUS; + extern "C" void init(Handle); +void +sigChldHandler(int sig) { + int status = 0; + pid_t res; + res = waitpid(CHILD_PID, &status, 0); + if (res != 0) { + CHILD_STATUS = WEXITSTATUS(status); + } +} + /** * PtyFork * pty.fork(file, args, env, cwd, cols, rows) @@ -211,6 +229,10 @@ PtyFork(const Arguments& args) { obj->Set(String::New("pid"), Number::New(pid)); obj->Set(String::New("pty"), String::New(name)); + CHILD_PID = pid; + CHILD_STATUS = -1; + signal(SIGCHLD, sigChldHandler); + return scope.Close(obj); } @@ -334,6 +356,21 @@ PtyGetProc(const Arguments& args) { return scope.Close(name_); } +/** + * PtyGetStatus + * Child process exit code + * pty.status() + */ + +static Handle +PtyGetStatus(const Arguments& args) { + HandleScope scope; + + Local num = Number::New(CHILD_STATUS); + return scope.Close(num); +} + + /** * execvpe */ @@ -557,6 +594,7 @@ init(Handle target) { NODE_SET_METHOD(target, "open", PtyOpen); NODE_SET_METHOD(target, "resize", PtyResize); NODE_SET_METHOD(target, "process", PtyGetProc); + NODE_SET_METHOD(target, "status", PtyGetStatus); } NODE_MODULE(pty, init) From 0e08d5dbe64528af7440b7165729177ea7f89e32 Mon Sep 17 00:00:00 2001 From: Niall O'Higgins Date: Mon, 29 Jul 2013 11:38:54 -0700 Subject: [PATCH 02/11] use a PID -> exit code map to avoid race conditions. --- lib/pty.js | 2 +- src/unix/pty.cc | 37 +++++++++++++++++++++++-------------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/lib/pty.js b/lib/pty.js index 7514854..cf2b05c 100644 --- a/lib/pty.js +++ b/lib/pty.js @@ -304,7 +304,7 @@ Terminal.prototype.__defineGetter__('process', function() { }); Terminal.prototype.__defineGetter__('status', function() { - return pty.status(); + return pty.status(this.pid); }); Terminal.prototype._close = function() { diff --git a/src/unix/pty.cc b/src/unix/pty.cc index c4e5dc5..40cb736 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -25,6 +25,7 @@ #include #include #include +#include /* forkpty */ /* http://www.gnu.org/software/gnulib/manual/html_node/forkpty.html */ @@ -97,19 +98,18 @@ pty_forkpty(int *, char *, const struct termios *, const struct winsize *); -static pid_t CHILD_PID; -static int CHILD_STATUS; +std::map pidMap; extern "C" void init(Handle); void -sigChldHandler(int sig) { +sigChldHandler(int sig, siginfo_t *sip, void *ctx) { int status = 0; pid_t res; - res = waitpid(CHILD_PID, &status, 0); + res = waitpid(sip->si_pid, &status, 0); if (res != 0) { - CHILD_STATUS = WEXITSTATUS(status); + pidMap[sip->si_pid] = WEXITSTATUS(status); } } @@ -228,10 +228,12 @@ PtyFork(const Arguments& args) { obj->Set(String::New("fd"), Number::New(master)); obj->Set(String::New("pid"), Number::New(pid)); obj->Set(String::New("pty"), String::New(name)); - - CHILD_PID = pid; - CHILD_STATUS = -1; - signal(SIGCHLD, sigChldHandler); + obj->Set(String::New("status"), Undefined()); + struct sigaction action; + memset (&action, '\0', sizeof(action)); + action.sa_sigaction = sigChldHandler; + action.sa_flags = SA_SIGINFO; + sigaction(SIGCHLD, &action, NULL); return scope.Close(obj); } @@ -358,18 +360,25 @@ PtyGetProc(const Arguments& args) { /** * PtyGetStatus - * Child process exit code - * pty.status() + * Foreground Process Name + * pty.status(pid) */ static Handle PtyGetStatus(const Arguments& args) { HandleScope scope; - Local num = Number::New(CHILD_STATUS); - return scope.Close(num); -} + if (args.Length() != 1 + || !args[0]->IsNumber()) { + return ThrowException(Exception::Error( + String::New("Usage: pty.status(pid)"))); + } + + int pid = args[0]->IntegerValue(); + Local statusCode = Number::New(pidMap[pid]); + return scope.Close(statusCode); +} /** * execvpe From 11fb6fa9341adbb36bf557792eef370e25972f8d Mon Sep 17 00:00:00 2001 From: Niall O'Higgins Date: Mon, 29 Jul 2013 11:41:34 -0700 Subject: [PATCH 03/11] demonstrate fetching exit status --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 1fbb78d..6fcbd5c 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,10 @@ term.on('data', function(data) { console.log(data); }); +term.on('close', function() { + console.log('exit status: %d', term.status); +}); + term.write('ls\r'); term.resize(100, 40); term.write('ls /\r'); From a4d9d518046eb53e80a1791bd772afe153a726a0 Mon Sep 17 00:00:00 2001 From: Niall O'Higgins Date: Mon, 29 Jul 2013 11:47:25 -0700 Subject: [PATCH 04/11] zap status property which krept in. --- src/unix/pty.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 40cb736..8a5d537 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -228,7 +228,6 @@ PtyFork(const Arguments& args) { obj->Set(String::New("fd"), Number::New(master)); obj->Set(String::New("pid"), Number::New(pid)); obj->Set(String::New("pty"), String::New(name)); - obj->Set(String::New("status"), Undefined()); struct sigaction action; memset (&action, '\0', sizeof(action)); action.sa_sigaction = sigChldHandler; From 2639c926028a1eac64d924e2e1f0f3319cc9d896 Mon Sep 17 00:00:00 2001 From: Niall O'Higgins Date: Fri, 2 Aug 2013 16:46:50 -0700 Subject: [PATCH 05/11] call node/libuv's SIGCHLD handler. --- src/unix/pty.cc | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 8a5d537..44c38db 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -103,6 +103,8 @@ std::map pidMap; extern "C" void init(Handle); +static void (*node_sighandler)(int) = NULL; + void sigChldHandler(int sig, siginfo_t *sip, void *ctx) { int status = 0; @@ -111,6 +113,10 @@ sigChldHandler(int sig, siginfo_t *sip, void *ctx) { if (res != 0) { pidMap[sip->si_pid] = WEXITSTATUS(status); } + // Can only have one SIGCHLD handler at a time, so we need to call node/libuv's handler. + if (node_sighandler) { + node_sighandler(sig); + } } /** @@ -228,11 +234,6 @@ PtyFork(const Arguments& args) { obj->Set(String::New("fd"), Number::New(master)); obj->Set(String::New("pid"), Number::New(pid)); obj->Set(String::New("pty"), String::New(name)); - struct sigaction action; - memset (&action, '\0', sizeof(action)); - action.sa_sigaction = sigChldHandler; - action.sa_flags = SA_SIGINFO; - sigaction(SIGCHLD, &action, NULL); return scope.Close(obj); } @@ -598,6 +599,17 @@ pty_forkpty(int *amaster, char *name, extern "C" void init(Handle target) { HandleScope scope; + // retrieve node/libuv's SIGCHLD handler. + struct sigaction node_action; + node_action.sa_flags = 0; + sigaction(SIGCHLD, NULL, &node_action); + node_sighandler = node_action.sa_handler; + struct sigaction action; + memset (&action, '\0', sizeof(action)); + action.sa_sigaction = sigChldHandler; + action.sa_flags = SA_SIGINFO; + // set new SIGCHLD handler. this will call node/libuv's handler at the end. + sigaction(SIGCHLD, &action, NULL); NODE_SET_METHOD(target, "fork", PtyFork); NODE_SET_METHOD(target, "open", PtyOpen); NODE_SET_METHOD(target, "resize", PtyResize); From 313e27e3371b9b87566d52691dabacd731ba8323 Mon Sep 17 00:00:00 2001 From: Jared Forsyth Date: Fri, 2 Aug 2013 18:39:21 -0600 Subject: [PATCH 06/11] fixing interoperability with native child_process. also added tests --- src/unix/pty.cc | 14 ++++++++------ test/index.js | 32 ++++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 44c38db..7902241 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -109,12 +109,13 @@ void sigChldHandler(int sig, siginfo_t *sip, void *ctx) { int status = 0; pid_t res; - res = waitpid(sip->si_pid, &status, 0); - if (res != 0) { - pidMap[sip->si_pid] = WEXITSTATUS(status); - } - // Can only have one SIGCHLD handler at a time, so we need to call node/libuv's handler. - if (node_sighandler) { + if (pidMap[sip->si_pid] == -302) { // this is one of ours + res = waitpid(sip->si_pid, &status, 0); + if (res != 0) { + pidMap[sip->si_pid] = WEXITSTATUS(status); + } + } else if (node_sighandler) { + // Can only have one SIGCHLD handler at a time, so we need to call node/libuv's handler. node_sighandler(sig); } } @@ -234,6 +235,7 @@ PtyFork(const Arguments& args) { obj->Set(String::New("fd"), Number::New(master)); obj->Set(String::New("pid"), Number::New(pid)); obj->Set(String::New("pty"), String::New(name)); + pidMap[pid] = -302; return scope.Close(obj); } diff --git a/test/index.js b/test/index.js index 7915cb7..517ba5d 100644 --- a/test/index.js +++ b/test/index.js @@ -7,6 +7,7 @@ var tests = [ name: 'should be correctly setup', command: [ 'children/void.js' ], options: { cwd: __dirname }, + exitCode: 0, test: function () { assert.equal(this.file, process.execPath); } @@ -14,6 +15,7 @@ var tests = [ name: 'should support stdin', command: [ 'children/stdin.js' ], options: { cwd: __dirname }, + exitCode: 0, test: function () { this.write('☃'); } @@ -21,6 +23,7 @@ var tests = [ name: 'should support resize', command: [ 'children/resize.js' ], options: { cwd: __dirname }, + exitCode: 0, test: function () { this.resize(100, 100); } @@ -28,7 +31,14 @@ var tests = [ name: 'should change uid/gid', command: [ 'children/uidgid.js' ], options: { cwd: __dirname, uid: 777, gid: 777 }, + exitCode: 0, test: function () {} + }, { + name: 'should report exitCode', + command: [ 'children/exitCode.js' ], + options: { cwd: __dirname }, + test: function () {}, + exitCode: 5 } ]; @@ -42,14 +52,8 @@ describe('Pty', function() { var term = pty.fork(process.execPath, testCase.command, testCase.options); term.pipe(process.stderr); - // any output is considered failure. this is only a workaround - // until the actual error code is passed through - var count = 0; - term.on('data', function (data) { - count++; - }); - term.on('exit', function () { - assert.equal(count, 0); + term.on('close', function () { + assert.equal(term.status, testCase.exitCode); done(); }); @@ -58,3 +62,15 @@ describe('Pty', function() { }); }); }); + +describe('The SIGCHLD handler', function () { + it('should not interfere with child_process', function (done) { + this.timeout(500); + var spawn = require('child_process').spawn; + var proc = spawn('false') + proc.on('close', function (code) { + assert.equal(code, 1); + done(); + }); + }); +}); From 1586aae0c19246e931d2d79d2b99208828532449 Mon Sep 17 00:00:00 2001 From: Niall O'Higgins Date: Sun, 4 Aug 2013 13:25:38 -0700 Subject: [PATCH 07/11] must set exit status correctly --- src/unix/pty.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 7902241..0056dae 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -111,9 +111,7 @@ sigChldHandler(int sig, siginfo_t *sip, void *ctx) { pid_t res; if (pidMap[sip->si_pid] == -302) { // this is one of ours res = waitpid(sip->si_pid, &status, 0); - if (res != 0) { - pidMap[sip->si_pid] = WEXITSTATUS(status); - } + pidMap[sip->si_pid] = WEXITSTATUS(status); } else if (node_sighandler) { // Can only have one SIGCHLD handler at a time, so we need to call node/libuv's handler. node_sighandler(sig); From 38a6b070eda591c4cf6339aafb040d3d7d96e96d Mon Sep 17 00:00:00 2001 From: Niall O'Higgins Date: Sun, 4 Aug 2013 13:31:02 -0700 Subject: [PATCH 08/11] set SA_NOCLDSTOP flag for SIGCHLD handler --- src/unix/pty.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 0056dae..68747b0 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -608,6 +608,7 @@ init(Handle target) { memset (&action, '\0', sizeof(action)); action.sa_sigaction = sigChldHandler; action.sa_flags = SA_SIGINFO; + action.sa_flags |= SA_NOCLDSTOP; // set new SIGCHLD handler. this will call node/libuv's handler at the end. sigaction(SIGCHLD, &action, NULL); NODE_SET_METHOD(target, "fork", PtyFork); From e7e82c68ff94b75516e12c66bbcff773c46742dc Mon Sep 17 00:00:00 2001 From: Jared Forsyth Date: Sun, 4 Aug 2013 18:20:51 -0600 Subject: [PATCH 09/11] node 0.10 compatibility --- src/unix/pty.cc | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 68747b0..acfd5bb 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -118,6 +118,25 @@ sigChldHandler(int sig, siginfo_t *sip, void *ctx) { } } +void +check_sigchld() { + // retrieve node/libuv's SIGCHLD handler. + struct sigaction node_action; + node_action.sa_flags = 0; + sigaction(SIGCHLD, NULL, &node_action); + if (node_action.sa_sigaction == sigChldHandler) { + return; // it's already installed + } + node_sighandler = node_action.sa_handler; + struct sigaction action; + memset (&action, '\0', sizeof(action)); + action.sa_sigaction = sigChldHandler; + action.sa_flags = SA_SIGINFO; + action.sa_flags |= SA_NOCLDSTOP; + // set new SIGCHLD handler. this will call node/libuv's handler at the end. + sigaction(SIGCHLD, &action, NULL); +} + /** * PtyFork * pty.fork(file, args, env, cwd, cols, rows) @@ -234,6 +253,7 @@ PtyFork(const Arguments& args) { obj->Set(String::New("pid"), Number::New(pid)); obj->Set(String::New("pty"), String::New(name)); pidMap[pid] = -302; + check_sigchld(); return scope.Close(obj); } @@ -592,6 +612,7 @@ pty_forkpty(int *amaster, char *name, #endif } + /** * Init */ @@ -599,18 +620,7 @@ pty_forkpty(int *amaster, char *name, extern "C" void init(Handle target) { HandleScope scope; - // retrieve node/libuv's SIGCHLD handler. - struct sigaction node_action; - node_action.sa_flags = 0; - sigaction(SIGCHLD, NULL, &node_action); - node_sighandler = node_action.sa_handler; - struct sigaction action; - memset (&action, '\0', sizeof(action)); - action.sa_sigaction = sigChldHandler; - action.sa_flags = SA_SIGINFO; - action.sa_flags |= SA_NOCLDSTOP; - // set new SIGCHLD handler. this will call node/libuv's handler at the end. - sigaction(SIGCHLD, &action, NULL); + check_sigchld(); NODE_SET_METHOD(target, "fork", PtyFork); NODE_SET_METHOD(target, "open", PtyOpen); NODE_SET_METHOD(target, "resize", PtyResize); From c7e72a5523ec603c828ca6ad6ec0433bfc9e83b6 Mon Sep 17 00:00:00 2001 From: Jared Forsyth Date: Sun, 4 Aug 2013 18:21:47 -0600 Subject: [PATCH 10/11] adding travis --- .travis.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..df63076 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,4 @@ +language: node_js +node_js: + - "0.10" + - "0.8" From e43d925445810b117d8b20bf90de384ea7308a62 Mon Sep 17 00:00:00 2001 From: Jared Forsyth Date: Sun, 4 Aug 2013 18:21:59 -0600 Subject: [PATCH 11/11] missed test file --- test/children/exitCode.js | 1 + 1 file changed, 1 insertion(+) create mode 100644 test/children/exitCode.js diff --git a/test/children/exitCode.js b/test/children/exitCode.js new file mode 100644 index 0000000..bd95aba --- /dev/null +++ b/test/children/exitCode.js @@ -0,0 +1 @@ +process.exit(5);