Node: child_process.spawn sh not killable

[paulpflug]

Comming from here:
https://github.com/keithamus/parallelshell/issues/22

(btw: Usecase for #1009)

Everytime I need to spawn a process I do it like this

if (process.platform === 'win32') {
    sh = 'cmd';
    shFlag = '/c';
} else {
    sh = 'sh';
    shFlag = '-c';
}
var child = spawn(sh,[shFlag,cmd], {
  cwd: process.cwd,
  env: process.env,
  stdio: ['pipe', process.stdout, process.stderr]
})

The problem: child is unkillable on unix.
Example in coffee-script:

spawn = require("child_process").spawn
child = spawn "sh", ["-c", "node -e 'setTimeout(function(){},10000);'"]
child.on "close", process.exit
child.kill()
child.kill("SIGINT")
child.kill("SIGTERM")
child.kill("SIGHUP")
spawn "sh",["-c","kill -TERM "+child.pid]
spawn "sh",["-c","kill -INT "+child.pid]
spawn "sh",["-c","kill -HUP "+child.pid]

(on windows it works fine with sh replaced by cmd)
Even when I exit the process with process.exit(), the child stays alive.

Workaround I found:

spawn = require("child_process").spawn
child = spawn "sh", ["-c", "node -e 'setTimeout(function(){},10000);'"], detached: true
child.on "close", process.exit
spawn "sh",["-c","kill -INT -"+child.pid]

Killing by pgid works, by pid works not. Sending ^C from console works also, I assume it uses pgid always.

[bnoordhuis]

I don't think that's a bug. Your example sends signals to sh, not node. sh may elect to ignore SIGINT, SIGTERM, etc.

The reason it works with { detached: true } is that it creates a new session group, i.e., it calls setsid(2). In that case, PID == PGID, and sending a signal will deliver it to all processes in the group.

[paulpflug]

But with detached: false I would expect sh to get cleaned up on process.exit()

That should be the point of the detached option?

[bnoordhuis]

I think you have the wrong idea about processes and process groups. On exit, child processes get reparented, they're not forcibly killed by io.js or the kernel.

{ detached: true } turns the child process into a group leader, that's really all it does.

[paulpflug]

From the node docs: options.detached

This makes it possible for the child to continue running after the parent exits.

What I read from that sentence: if detached = false the child get force killed

On exit, child processes get reparented, they're not forcibly killed by io.js or the kernel.

What you say is, that the child is always continuing after the parent exits, regardless of detached
I would definitivly expect different behavior as a user.
At least, we have to alter the docs :smile:

[bnoordhuis]

The documentation for options.detached is not wrong as such, but it's not the whole story.

What you say is, that the child is always continuing after the parent exits, regardless of detached

Not quite. { detached: true } means, among other things, that the child process won't receive a SIGINT when you kill the parent with ^C.

/cc @nodejs/documentation - the documentation can probably be improved, although I'm not sure where to draw the line between inline documentation and pointing people to a UNIX process management primer.

[okdistribute]

@paulpflug what exact platform were you using to test this? (any more specific than unix?) I found the same problem, works on mac but not on ubuntu. Seems to close properly if I use /bin/bash instead, got the idea from #4432

[paulpflug]

@karissa ubuntu ;) I'm unsure of the reliability of bash

currently I live with using detached: process.platform != "win32" on spawn
and killing by gpid

if (process.platform != "win32") {
  spawn("sh", ["-c", "kill -INT -"+child.pid]);
  } else {
  child.kill("SIGINT");
}

I would really appreciate a more beautiful solution, but found none so far.
Thanks for pointing out, though.

[sam-github]

@paulpflug If you want to send a signal to an entire process group, using detached (on non-windows) to put the child and its descendants in a group is exactly what unix process gouping is intended for.

Your spawning of kill should be unnecessary, use process.kill(-child.pid, 'SIGINT') - this should work, and if it doesn't, file a bug report!

[paulpflug]

@sam-github that is cool, didn't know that. And it does work :+1:

I created a small wrapper: better-spawn

[cjihrig]

This isn't Node specific, but it might be something worth mentioning in the documentation.

[jasnell]

@nodejs/documentation

[eljefedelrodeodeljefe]

Though I agree with the general "processes on the different platforms" discussion, I think I can confirm a bug here:
Actually there could be a number of bugs. Most significant, likely only one, is that child.pid is off by -1 in the following example. Adding +1 manually fixes it. On OS X it works fine without the addition. Listening for SIGHUP is optional and only demonstrates, that sending and catching signals works.

'use strict'
const spawn = require('child_process').spawn

console.log('Spawning from:', process.pid)

let child = spawn('sh', ['-c',
  `node -e "

  process.on('SIGHUP', () => {console.log('received SIGHUP', process.exit())})

  setInterval(() => {
      console.log(process.pid, 'is alive')
    }, 500);"`
  ], {
    stdio: ['inherit', 'inherit', 'inherit']
  })

child.on('close', () => {
  console.log('caught child\'s exit')
  // exiting here should be unecessary
  process.exit()
})

setTimeout(() => {
  // child.kill()
  // child.kill('SIGINT')
  // child.kill('SIGTERM')
  // child.kill('SIGHUP')
  console.log(child.pid)
  process.kill(child.pid + 1, 'SIGHUP')
}, 2000)

This has the following output.

Spawning from: 32656
32662 'is alive'
32662 'is alive'
32662 'is alive'
32661
caught child's exit

• Linux ubuntu 3.13.0-83-generic #127-Ubuntu SMP Fri Mar 11 00:25:37 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
• node 5.9.0

[eljefedelrodeodeljefe]

Scratch what I said. It's the pid of sh that causes the offset. Hence it's only natural to increment by one. This then could only lead to such misunderstandings, also with the new shell option. Gonna crate a PR for spawning shells and kill on linux, if there are no objections to the contents in the PR.

For sh not being killable linux specific process implementation applies.

Here is the output of ps auxfor the above.

paralle+  3466  1.0  0.1 637396 15988 pts/2    Sl+  12:06   0:00 ./node app.js
paralle+  3474  0.0  0.0   4440   656 pts/2    S+   12:06   0:00 sh -c node -e "    process.on('SIGHUP', () => {con
paralle+  3475  0.5  0.1 641360 14988 pts/2    Sl+  12:06   0:00 node -e     process.on('SIGHUP', () => {console.lo
paralle+  3519  0.0  0.0  22640  1312 pts/1    R+   12:06   0:00 ps aux

[Knighton910]

+1 @eljefedelrodeodeljefe

Gonna crate a PR for spawning shells and kill on linux, if there are no objections to the contents in the PR.

[benjamingr]

Closed by PR by @eljefedelrodeodeljefe :tada:

[eljefedelrodeodeljefe]

Ping, everyone interested I have sent in a PR at libuv to land something we could implement as

let clds = process.children();
// => [ 5598, 5601]

clds.forEach((pid) => {
   process.kill(pid);
});

Happy for initial thoughts (in light of the problem here).

[stevemao]

@eljefedelrodeodeljefe can you link it here?

[eljefedelrodeodeljefe]

Sure. See above also or here libuv/libuv#836

[benjamingr]

+1 for adding the .children() part to the docs and to the NodeJS API.

[leo]

My god!

Thank you so much for bringing this up, guys. I've spent countless hours trying to figure out why my child process didn't receive these events – it's fixed now.

🙏