Skip to content

Commit

Permalink
On Linux, when killing the child process, kill all the descendents as…
Browse files Browse the repository at this point in the history
… well

This attempt to fix the problem described by the issue sindresorhus#96.
If a child process is killed, the descendents of that child process won't be killed as well. This happens on Linux but not on Windows [^1].
A solution is the "PID range hack" [^2] that uses the `detached` mode for spawning a process and then kills that child process by killing the PID group, using `process.kill(-pid)`.

*Implementation*

- added an internal option `killByPid` as a remained for the spawned child process that it will be `detached` and to kill it by PID
- expanded and moved to a separate function the routine to kill the spawned process to `killSpawned`
- the `ChildProcess#kill` method of the spawned child process will be replaced by the `killSpawned` routine, to kill by pid if necessary
- the `killSpawned` routine signals that the child process has been killed, if it has been killed by the pid

I checked and all the tests pass.

This implementation also consider the issue sindresorhus#115 and shouldn't interfere with the detached/cleanup fix.

[^1]: https://nodejs.org/api/child_process.html#child_process_subprocess_kill_signal
[^2]: https://azimi.me/2014/12/31/kill-child_process-node-js.html
  • Loading branch information
tomsotte committed Jan 17, 2019
1 parent 2210988 commit 5bdd55a
Showing 1 changed file with 30 additions and 6 deletions.
36 changes: 30 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ function handleArgs(command, args, options) {
encoding: 'utf8',
reject: true,
cleanup: true
}, parsed.options, {windowsHide: true});
}, parsed.options, {
windowsHide: true,
killByPid: false
});

// TODO: Remove in the next major release
if (options.stripEof === false) {
Expand All @@ -68,6 +71,14 @@ function handleArgs(command, args, options) {
options.cleanup = false;
}

if (process.platform !== 'win32') {
// #96
// Windows automatically kills every descendents of the child process
// On Linux (MacOS too?) we need to detach the process and kill by `sid`
options.detached = true;
options.killByPid = true;
}

if (process.platform === 'win32' && path.basename(parsed.command) === 'cmd.exe') {
// #116
parsed.args.unshift('/q');
Expand Down Expand Up @@ -212,11 +223,24 @@ module.exports = (command, args, options) => {
return Promise.reject(error);
}

let killed = false;

const killSpawned = signal => {
if (parsed.options.killByPid && spawned) {
// Kills the spawned process and its descendents using the pid range hack
// Source: https://azimi.me/2014/12/31/kill-child_process-node-js.html
process.kill(-spawned.pid, signal);
killed = true;
} else {
spawned.kill(signal);
}
};

spawned.kill = killSpawned;

let removeExitHandler;
if (parsed.options.cleanup) {
removeExitHandler = onExit(() => {
spawned.kill();
});
removeExitHandler = onExit(killSpawned);
}

let timeoutId = null;
Expand All @@ -237,7 +261,7 @@ module.exports = (command, args, options) => {
timeoutId = setTimeout(() => {
timeoutId = null;
timedOut = true;
spawned.kill(parsed.options.killSignal);
killSpawned(parsed.options.killSignal);
}, parsed.options.timeout);
}

Expand Down Expand Up @@ -289,7 +313,7 @@ module.exports = (command, args, options) => {
// TODO: missing some timeout logic for killed
// https://github.com/nodejs/node/blob/master/lib/child_process.js#L203
// error.killed = spawned.killed || killed;
error.killed = error.killed || spawned.killed;
error.killed = error.killed || spawned.killed || killed;

if (!parsed.options.reject) {
return error;
Expand Down

0 comments on commit 5bdd55a

Please sign in to comment.