From 5bdd55a167f69c346a6f8c44d9a8c7c7abec35ff 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] On Linux, when killing the child process, kill all the
 descendents as well

This 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].
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 #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 | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/index.js b/index.js
index 0610a581cc..3777e0c9f4 100644
--- a/index.js
+++ b/index.js
@@ -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) {
@@ -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');
@@ -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;
@@ -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 || killed;
 
 			if (!parsed.options.reject) {
 				return error;