-
-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ability to kill all descendents of the child process #96
Comments
… 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
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
Hi, I've tried to implement a possible solution by using the pid range hack. I think it integrates in what The solution I've implemented replaces:
It shouldn't and doesn't AFAIK interfere with the |
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
See: #170 (comment) |
Note that this is especially problematic when using The following: const subprocess = execa('sleep 200', {shell: true});
subprocess.kill();
await subprocess; Is the same as: const subprocess = execa('/bin/sh', ['-c', 'sleep 200']);
subprocess.kill();
await subprocess; This kills Note that both the |
FWIW, I use a possible workaround (of sorts...), posting it here for others who might have the same issue and come across this thread. But.. it ain't pretty, and YMMW for its usefulness: import execa, { ExecaChildProcess } from "execa"
import pidtree from "pidtree"
export type Execa2ChildProcess = ExecaChildProcess & { killTree: () => Promise<boolean> }
export function execa2Command (cmd: string, opts: { cwd?: string; shell?: boolean; } = {}): Execa2ChildProcess {
const p = execa.command(cmd, opts)
async function killTree (): Promise<boolean> {
let pids
try {
pids = await pidtree(p.pid, { root: true })
} catch (err) {
if (err.message === "No maching pid found") return true // *
throw err
}
pids.map(el => {
try {
process.kill(el)
} catch (err) {
if (err.code !== "ESRCH") throw err
}
})
p.kill()
return true
}
(p as any).killTree = killTree
return p as ExecerChildProcess
} * the error-message from pidtree is misspelled, PR simonepri/pidtree#11 attempts to fix that This solution won't win any prizes and is maybe provably wrong in key areas, but it at least satisfies my test-suite so I can move on in my tasks. |
This solution ( |
Here is a good way to do this https://www.npmjs.com/package/terminate const terminate = require('terminate');
const subprocess = execa("yarn", ["start"], {stdio: 'inherit', cwd: tempDir })
terminate(subprocess.pid, 'SIGINT', { timeout: 1000 }, () => {
terminate(subprocess.pid);
}); |
I was using const getPids = async pid => {
const { value: pids = [] } = await pReflect(pidtree(pid))
return pids.includes(pid) ? pids : [...pids, pid]
}
const pids = await getPids(pid)
pids.forEach(pid => {
try {
process.kill(pid, signal)
} catch (_) {} |
Relevant Node.js issue: nodejs/node#40438 |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
I had good luck with this method. I don't know if it si relevant but I only have to do this on Windows. While both on MacOS and Windows spawned processes are grouped correctly (as it can be seen by the task manager), in MacOS they are all terminated correctly when the main process is terminated, in Windows that is not the case. |
Although this issue has been open for almost 7 years now, this would still be a very valuable feature. Some of the points anyone implementing this should consider:
|
This is the way I'm doing this these days without using dependencies, just Node.js API, in the way @tomsotte (5 years ago!): https://github.com/Kikobeats/kill-process-group/blob/f32f97491f255a7b763a578c46b49da78b1485a6/src/index.js#L8-L33 |
This is indeed a problem. It would be great if there is a better solution. |
When I'm working with child processes, I sometimes end up with detached processes running indefinitely after I've killed the direct child, which can lead to blocked ports and memory leaks.
I sometimes fix this by using a module like ps-tree to find out all the descendants PIDs, and then send all of them a
kill
command, but it feels messy. It would be nice if execa could provide a clean, cross-platform API for this.Maybe something like this:
The text was updated successfully, but these errors were encountered: