Open callumlocke opened 7 years ago
Hi,
I've tried to implement a possible solution by using the pid range hack. I think it integrates in what execa
already provides, which is a better child_process
with cross-platform support. On Windows, when killing a process it would kill all of it descendents, but this does not happen on Linux (and apparently on macOS too).
The solution I've implemented replaces:
execa
kills the spawned process for the cleanup routineChildProcess#kill
methodIt shouldn't and doesn't AFAIK interfere with the detached
and cleanup
choice. But I'd like if someone can review and double-check it.
Note that this is especially problematic when using shell: true
.
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 /bin/sh
but not sleep 200
, which is a separate process spawned by /bin/sh
.
Note that both the shell: true
behavior and the fact that descendant child processes are not recursively killed are not specific issues to Execa, but to core child_process.spawn()
(that Execa is based upon).
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 https://github.com/simonepri/pidtree/pull/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 (pidtree
) started to be implemented in this PR. We first looked into build this into childProcess.kill()
. But it turns out it was big enough that we thought this should either be its own library or be built in core Node.js itself.
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 pidtree
and it works pretty well, plus the dependency size is small.
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: https://github.com/nodejs/node/issues/40438
await subprocess;
@ehmicky what you are doing here is a mistery, since when you can await
a variable ?
Promises are variables that can be awaited. subprocess
is a mixin of a Promise
and a child process. This might be off-topic, so I am going to hide those comments to keep the thread focused on terminating descendants of a child process.
I was using
pidtree
and it works pretty well, plus the dependency size is small.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 (_) {}
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:
ps
utility has different flags on all the following OSes/distributions: macOS, Linux, Amazon Linux, Alpine Linux, etc. Sometimes it is not even available.process.kill(-processGroup)
signal
option, the timeout
option, the cleanup
option, the maxBuffer
option, and also any error
event on childProcess
, childProcess.stdin
, childProcess.stdout
or childProcess.stderr
. One might want to terminate the process and its descendants in those cases. cleanup
option, the code will only work if synchronous.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
https://github.com/sindresorhus/execa/issues/96#issuecomment-776280798
https://github.com/sindresorhus/execa/issues/96#issuecomment-928268856 https://github.com/sindresorhus/execa/issues/96#issuecomment-1892613980
This is indeed a problem. It would be great if there is a better solution. The above is the solution I found from the discussion.
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: