From c16cc46689aec09869a2b0cbba62c11ff5d7de3d Mon Sep 17 00:00:00 2001
From: Tommaso Sotte <tommaso.sotte@gmail.com>
Date: Thu, 17 Jan 2019 23:43:46 +0100
Subject: [PATCH] When killing the child process, kill all the descendents as
 well

This is an attempt to fix the problem described by the issue #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 #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
---
 index.js | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/index.js b/index.js
index 0610a581cc..66bed0391d 100644
--- a/index.js
+++ b/index.js
@@ -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) {
@@ -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) {
@@ -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');
 	}
@@ -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;
@@ -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);
 	}
 
@@ -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;
@@ -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);