Skip to content

Commit c16cc46

Browse files
committed
When killing the child process, kill all the descendents as well
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
1 parent 2210988 commit c16cc46

File tree

1 file changed

+34
-7
lines changed

1 file changed

+34
-7
lines changed

index.js

+34-7
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ const onExit = require('signal-exit');
1111
const errname = require('./lib/errname');
1212
const stdio = require('./lib/stdio');
1313

14+
const isWin = process.platform === 'win32';
15+
1416
const TEN_MEGABYTES = 1000 * 1000 * 10;
1517

1618
function handleArgs(command, args, options) {
@@ -50,7 +52,9 @@ function handleArgs(command, args, options) {
5052
encoding: 'utf8',
5153
reject: true,
5254
cleanup: true
53-
}, parsed.options, {windowsHide: true});
55+
}, parsed.options, {
56+
windowsHide: true
57+
});
5458

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

71-
if (process.platform === 'win32' && path.basename(parsed.command) === 'cmd.exe') {
75+
if (!isWin) {
76+
// #96
77+
// Windows automatically kills every descendents of the child process
78+
// On Linux and macOS we need to detach the process and kill by pid range
79+
options.detached = true;
80+
}
81+
82+
if (isWin && path.basename(parsed.command) === 'cmd.exe') {
7283
// #116
7384
parsed.args.unshift('/q');
7485
}
@@ -212,11 +223,24 @@ module.exports = (command, args, options) => {
212223
return Promise.reject(error);
213224
}
214225

226+
let killedByPid = false;
227+
228+
spawned._kill = spawned.kill;
229+
230+
const killSpawned = signal => {
231+
if (process.platform === 'win32') {
232+
spawned._kill(signal);
233+
} else {
234+
// Kills the spawned process and its descendents using the pid range hack
235+
// Source: https://azimi.me/2014/12/31/kill-child_process-node-js.html
236+
process.kill(-spawned.pid, signal);
237+
killedByPid = true;
238+
}
239+
};
240+
215241
let removeExitHandler;
216242
if (parsed.options.cleanup) {
217-
removeExitHandler = onExit(() => {
218-
spawned.kill();
219-
});
243+
removeExitHandler = onExit(killSpawned);
220244
}
221245

222246
let timeoutId = null;
@@ -237,7 +261,7 @@ module.exports = (command, args, options) => {
237261
timeoutId = setTimeout(() => {
238262
timeoutId = null;
239263
timedOut = true;
240-
spawned.kill(parsed.options.killSignal);
264+
killSpawned(parsed.options.killSignal);
241265
}, parsed.options.timeout);
242266
}
243267

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

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

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

339+
// Enhance the `ChildProcess#kill` to ensure killing the process and its descendents
340+
spawned.kill = killSpawned;
341+
315342
handleInput(spawned, parsed.options.input);
316343

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

0 commit comments

Comments
 (0)