Skip to content

Commit

Permalink
When killing the child process, kill all the descendents as well
Browse files Browse the repository at this point in the history
This is an 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].
The adopted 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)`, effectively killing all the descendents.

*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 considers 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 20, 2019
1 parent 2210988 commit c16cc46
Showing 1 changed file with 34 additions and 7 deletions.
41 changes: 34 additions & 7 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const onExit = require('signal-exit');
const errname = require('./lib/errname');
const stdio = require('./lib/stdio');

const isWin = process.platform === 'win32';

const TEN_MEGABYTES = 1000 * 1000 * 10;

function handleArgs(command, args, options) {
Expand Down Expand Up @@ -50,7 +52,9 @@ function handleArgs(command, args, options) {
encoding: 'utf8',
reject: true,
cleanup: true
}, parsed.options, {windowsHide: true});
}, parsed.options, {
windowsHide: true
});

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

if (process.platform === 'win32' && path.basename(parsed.command) === 'cmd.exe') {
if (!isWin) {
// #96
// Windows automatically kills every descendents of the child process
// On Linux and macOS we need to detach the process and kill by pid range
options.detached = true;
}

if (isWin && 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 killedByPid = false;

spawned._kill = spawned.kill;

const killSpawned = signal => {
if (process.platform === 'win32') {
spawned._kill(signal);
} else {
// 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);
killedByPid = true;
}
};

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 || killedByPid;

if (!parsed.options.reject) {
return error;
Expand All @@ -312,6 +336,9 @@ module.exports = (command, args, options) => {

crossSpawn._enoent.hookChildProcess(spawned, parsed.parsed);

// Enhance the `ChildProcess#kill` to ensure killing the process and its descendents
spawned.kill = killSpawned;

handleInput(spawned, parsed.options.input);

spawned.then = (onfulfilled, onrejected) => handlePromise().then(onfulfilled, onrejected);
Expand Down

0 comments on commit c16cc46

Please sign in to comment.