Add support for .on('exit', ...) returning process exit status

[joshwilsdon]

This should resolve #28 and adds the ability to get the exit code and signal (if killed by signal) when a pty spawned process exits.

[psaia]

Any updates on this? Would be awesome to have merged.

[javruben]

@chjj We need the exit code that this PR implements to make the Cloud9 installer function properly. Would you have time to take a look? Thanks!

[tobiastom]

I would be interested into this as well.

[amshali]

Is there anyway we can get some traction on this? I would be willing to fix this with some guidance on what is the best way to do it. Looking at the issues, there are a couple of ways to go about it. @chjj any thought?

[chjj]

Sorry, this one flew totally under the radar for me. This handles SIGCHLD via libuv functions? Am I seeing that right? That's exactly what we needed if that's the case. Calling signal(SIGCHLD, callback) ourselves caused some dissonance between pty.cc and libuv.

[chjj]

Merging this for now. If this handles SIGCHLD the libuv way and fixes the zombie process issue, I'm all for it. This was the problem I had: binding to SIGCHLD the traditional way caused issues with libuv.

[chjj]

I'm getting a segfault on pty exit. node v0.12.7

[chjj]

That last commit seemed to fix it.

[chjj]

Sorry, the commit above this comment ^

[chjj]

The next step is to figure out how compatible this is with previous node/libuv versions.

[amshali]

Is it possible to issue a release which includes this? Thank you.

[chjj]

@amshali, yes, but I want to test it on multiple OSes and node versions first. Give it a day or two.

[chjj]

Also, feel free to test on whatever platform you're using and let us know.

[chjj]

So far works on node v0.12.x on Arch Linux and OSX.

[chjj]

Getting some bizarre side effects on v0.12.5 (Linux), but v0.12.7 seems fine.

[chjj]

I take it back. v0.12.5 seems fine now.

[chjj]

Build error on v0.10.7 (Linux):

make[2]: Entering directory '/home/user/work/node_modules/pty.js/build'
  CXX(target) Release/obj.target/pty/src/unix/pty.o
../src/unix/pty.cc: In function ‘v8::Handle<v8::Value> PtyFork(const v8::Arguments&)’:
../src/unix/pty.cc:282:74: error: invalid conversion from ‘void (*)(uv_async_t*) {aka void (*)(uv_async_s*)}’ to ‘uv_async_cb {aka void (*)(uv_async_s*, int)}’ [-fpermissive]
         uv_async_init(uv_default_loop(), &baton->async, after_wait_on_pid);
                                                                          ^
In file included from ../node_modules/nan/nan.h:23:0,
                 from ../src/unix/pty.cc:16:
/home/user/.node-gyp/0.10.17/deps/uv/include/uv.h:1186:15: note:   initializing argument 3 of ‘int uv_async_init(uv_loop_t*, uv_async_t*, uv_async_cb)’
 UV_EXTERN int uv_async_init(uv_loop_t*, uv_async_t* async,
               ^
pty.target.mk:83: recipe for target 'Release/obj.target/pty/src/unix/pty.o' failed
make[2]: *** [Release/obj.target/pty/src/unix/pty.o] Error 1
make[2]: Leaving directory '/home/user/work/node_modules/pty.js/build'

Looks like the uv api changed a bit between versions. We can account for that I suppose.

[chjj]

Added a version check to fix the arguments for past versions of uv_async_init.

[chjj]

Fixed node v0.10.7. Node v0.8.21 also works.

[chjj]

pty.js v0.2.9 published, complete with exit codes and no zombies.

[amshali]

Thanks a bunch!!! :)

[tobiastom]

Thank you. Can't wait to test it.

Would you mind posting a minimal test case, then I could test it on FreeBSD and OS X El Captain.

Am 31.07.2015 um 07:01 schrieb Christopher Jeffrey (JJ) notifications@github.com:

pty.js v0.2.9 published, complete with exit codes and no zombies.

—
Reply to this email directly or view it on GitHub.

[chjj]

@tobiastom, here's a simple test for now that you can copy and paste to see the exit status:

var pty = require('pty.js');

var term = pty.spawn('sleep', ['3'], {
  name: 'xterm-color',
  cols: 80,
  rows: 30,
  cwd: process.env.HOME,
  env: process.env
});

term.on('exit', function(code) {
  console.error('CODE: ' + code);
});

OR:

var pty = require('pty.js');

var term = pty.spawn('bash', ['-c', 'echo 1; sleep 3; echo 2; exit 1'], {
  name: 'xterm-color',
  cols: 80,
  rows: 30,
  cwd: process.env.HOME,
  env: process.env
});

term.on('data', function(data) {
  process.stderr.write(data);
});

term.on('exit', function(code) {
  console.error('CODE: ' + code);
});

[tobiastom]

@chjj Works great for me on:

• FreeBSD 10.1 with node v0.12.7
• OS X 10.10.4 with node v0.12.7
• OS X 10.11 (beta) with node v0.12.4

Thanks again for adding this functionality!

[chjj]

Remember to thank @joshwilsdon: he figured out a way to do this without overriding node's SIGCHLD handler, which was a huge issue in the first place.

[tobiastom]

Oh, I missed that the pull request originated from him.

Thanks @joshwilsdon. You made my day. :)

[chjj]

Fixed a few issues:

• Due to a race condition, sometimes we would get an assertion error because the handle.data was freed before UV_CLOSING was set on the flags: node: ../deps/uv/src/unix/core.c:210: uv__finish_close: Assertion 'handle->flags & UV_CLOSING' failed. The solution was to delete handle.data in the uv_close() callback instead.
• Due to another race condition, sometimes the last data event would be emitted after exit. It now waits for the socket to be destroyed before emitting exit.

[chjj]

It would be wise to test these fixes on all platforms and versions again.

[chjj]

Also, it looks like in node v0.8.x, waitpid is already handled somewhere else. errno is ECHILD. I've accounted for this, and everything will work, but node v0.8.x will always get an exit status of 0.

[chjj]

pty.js v0.2.10 published. Please test.

[tobiastom]

@chjj 0.2.11 seems to work on all my tested platforms from above.

[chjj]

@tobiastom, good news.

And I have another small improvement for this I'm going to publish tonight.

It looks like every platform from node v0.8.x onward should work at least.

[sandrewh]

In order to have the shell pass the exit code through to the term.on('exit', function(code){...});handler, I had to do something like...

pty.spawn('bash', ['-c', /path/to/my/app; exit $?'], {