process.on('exit') is disabled after spawn a child process.

[zhaoyi0113]

I am using node-pty to spawn a child process as below:

const { spawn } = require('node-pty');

const handleShutdown = (e, n) => {
  console.log('exit', e, n);
  process.exit(e);
};
process.on('exit', handleShutdown);
process.on('SIGINT', handleShutdown);
const p = spawn('mongo', ['mongodb://localhost:27017/admin']);

when I terminate the node process, the handleShutdown is not being called. But if I move the process.on after spawn, the handleShutdown is called on termination. I think the spawn override process listeners for some reasons. How can I fix that?

[Tyriar]

@zhaoyi0113 what platform?

[zhaoyi0113]

I am on Mac 10.13.3, node 8.9.1. Thanks.

[zhaoyi0113]

I did some more tests on that and found if there is a process.on before spawn, the process.on listeners are never being called no matter whether you call it after spawn or not. Take below code as an example,

const { spawn } = require('node-pty');

const handleShutdown = (e, n) => {
  console.log('exit', e, n);
  process.exit(e);
};
process.on('exit', handleShutdown);
process.on('SIGINT', handleShutdown);
process.on('SIGHUP', handleShutdown);
const p = spawn('mongo', ['mongodb://localhost:27017/admin']);
p.on('data', d => console.log(d));
p.write('show dbs\n');
process.on('exit', handleShutdown);
process.on('SIGINT', handleShutdown);
process.on('SIGHUP', handleShutdown);

The handleShutdown is not called on terminating the process even I added tham after spawn.

[Tyriar]

Isn't this because you're listening to exit and signals on the outer node process, not p?

[zhaoyi0113]

Yes, this is what I want to listen. I want to listen on the exit of the node process instead of spawn process. But I found once I spawn a process my listener doesn't work

[Tyriar]

I did some more tests on that and found if there is a process.on before spawn, the process.on listeners are never being called no matter whether you call it after spawn or not.

You have a reference to p so I think this is expected. To my knowledge node's process will only exit when it's sure there's nothing left to do. You could just listen to exit on p.

p.on('exit', handleShutdown);

[zhaoyi0113]

In my application, it will spawn many processes and I have exit listener listening to each of them (p). During my application lifetime, these child processes will be created and exit.

But I also need a listener who is listening to the node process exit event and do some logic there. I wonder why the spawn disable the outer process listeners?

[zhaoyi0113]

@Tyriar Could you let me know why you closed this issue? It is an obvious issue in node-pty. Let me know if you need more explanation or I can create a reproducible repo for you to test. Thanks.

[Tyriar]

@zhaoyi0113 because I don't think it is an obvious issue with node-pty. I don't know enough about node's lifecycle to know for sure but I think listeners/resources are sticking around so node doesn't think it should exit. You could try release the reference to p and/or call p.destroy() after you're done with the process. I think you should be managing the lifecycle of your application though, not leaving it up to the outer node process; when all x processes have finished you can call process.exit(0) to exit the process.

[zhaoyi0113]

@Tyriar My case is that I added an exit listener on node outer process to do some cleanup work. This listener is called when the outer process exit or crash or is killed for some reasons. It works fine until I spawn a node-pty process inside my node process. The node-pty child process removes the listeners I added to the outer process. So when my process crash or is killed, these listeners are not called therefore I don't have a chance to do the cleanup work. I understand that you want me to listen on the node-pty process exit listener but it is not the good place because I need to spawn and kill many node-pty processes during my application is running.

[Tyriar]

@zhaoyi0113 why can't you keep a stack of processes and trigger clean up/process.exit when the stack reaches 0? (or when you're done)

Also make sure you try this:

const { spawn } = require('node-pty');

const handleShutdown = (e, n) => {
  console.log('exit', e, n);
  process.exit(e);
};
process.on('exit', handleShutdown);
process.on('SIGINT', handleShutdown);
let p = spawn('mongo', ['mongodb://localhost:27017/admin']);
p.on('exit', () => {
  p.destroy();
  delete p;
});

Having said all this, I would accept PRs to better clean up anything inside a Terminal when you call destroy. In order to keep issues manageable I want to keep this closed though.

[zhaoyi0113]

@Tyriar The problem is that once I spawn a new sub process through node-pty, the process.on('exit') is not being called when the outer process exits. Any my clean up code has to be run when the outer process exits but not sub-process exit.

[shir]

@Tyriar have exactly same issue. I have node process which manages a lot of child processed which are spawned via node-pty. And I want to kill all this process (and some other jobs) on Ctrl+C and added process.on('SIGINT') handle. But this handler is not called if any child process is spawned via node-pty. Issue should be reopened.

[Tyriar]

@shir handle the exit event on the pty:

pty.on('exit', (code, signal) => {
  ...
});

[zhaoyi0113]

@Tyriar I think the problem is how to listen on process exit rather than listening to pty exit.

[Tyriar]

I run this piece of code and it exits correctly on Linux?

var os = require('os');
var pty = require('node-pty');

var shell = os.platform() === 'win32' ? 'powershell.exe' : 'bash';

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

ptyProcess.on('data', function(data) {
  console.log(data);
});

ptyProcess.write('ls\r');
ptyProcess.write('exit\r');
process.on('exit', c => console.log('exit ' + c));

[zhaoyi0113]

If you remove the line ptyProcess.write('exit\r'); and kill the process by ctrl + c. You will not see exit get printed.

[Tyriar]

@zhaoyi0113 works for me:

process.on('SIGINT', () => {
  console.log('cancel SIGINT');
});

[zhaoyi0113]

@Tyriar The problem happens if you add this listener before spawn pty process. Below code doesn't work for me on both ubuntu and mac.

var os = require('os');
var pty = require('node-pty');

var shell = os.platform() === 'win32'
  ? 'powershell.exe'
  : 'bash';

process.on('exit', c => console.log('exit ' + c));
process.on('SIGINT', c => {
  console.log('exit ' + c);
  process.exit();
});

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

ptyProcess.on('data', function (data) {
  console.log(data);
});

ptyProcess.write('ls\r');
// ptyProcess.write('exit\r');

[Tyriar]

Not sure what could be causing that, interaction with process is fairly limited

[danielrichman]

./node-pty/src/unix/pty.cc PtyFork calls signal(SIGINT, SIG_DFL) before forking, removing the signal handler in the parent process that was installed before then.

[Tyriar]

That line was added in 56fc4a4 and is not present in the original pty.js that this was forked off of. I also notice the man page says signal shouldn't be used:

The behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux. Avoid its use: use sigaction(2) instead. See Portability below.

@jerch any insights here? I haven't touched this code path personally.

[jerch]

Upps yes, signal is not standardized between different POSIX systems. sigaction is the way to go.

[danielrichman]

sigaction or signal, I still think you need to move whatever that line becomes to "after" the fork.

[jerch]

Not sure why it is there in the first place, all I know is that libuv's event loop and callback system was not fork save in earlier releases, but they worked on a fix around node v6. I think we should have a look at libuv's spawn implementation to decide where this should go or whether it can be removed.

edit: Btw there is another signal related issue - openpty is not guaranteed to be signal save. A robust impl would need to store the actual sigmask, reset it, call openpty and reestablish the saved sigmask.

[fpronto]

+1 here
DeepinOS 15.6
node v8.11.3

this happens even when all the processes from node-pty are killed

[jerch]

Tried to come up with a signal safe version in #218.
@zhaoyi0113 could you check if the problem remains with that version?

[zhaoyi0113]

@jerch Is the version released? It happens on the version "node-pty": "^0.7.6"

[jerch]

Nope still in PR stage, just wrote a few tests, not sure they will cover it correctly (hard to test since SIGINT will just end the mocha process).

[Battochon]

Hello,

I was banging my head over this problem and thanks, I saw this issue. Any update on this please? Help would be appreciated.

[Jesse-Schokker]

Any fixes?