Kill all descendants tree of a process #34
Conversation
When `tree` option is enabled the process specified by the input, and its descendants, will be killed. To kill the whole tree it has been added and used the module `tree-kill`. To handle a process name as input, which can match to multiple processes, it has been added the function `getPidsFromInput`, which uses `ps-list` to get all the pids by the proces name.
Various errors for pathing for fixtures has been corrected, mostly caused by `child_process.spawn`. On Windows it's not possible to rename a process in the task list via JS, so the test to kill tree by name has been disabled for Windows. On Windows, to taskkill a tree `force` must be enabled.
| }); | ||
| } | ||
|
|
||
| // eslint-disable-next-line ava/test-ended |
There was a problem hiding this comment.
I've disabled eslint for those line because the rule ava/test-ended seems to not correctly report the usage of t.end() when using a macro. Later on I may report as an issue to ava repo.
|
Can you fix the merge conflict? |
|
See pkrumins/node-tree-kill#20, maybe we should use |
| // eslint-disable-next-line ava/test-ended | ||
| test.cb('kill all descendants tree by pid', testKillDescendant); | ||
|
|
||
| if (process.platform !== 'win32') { |
There was a problem hiding this comment.
Why is this not tested on Windows?
There was a problem hiding this comment.
To test killing a tree of processes I need a parent process that can spawn a descendant.
To test killing a process by its name I need a fixture that can change name, otherwise I'm not able to pinpoint that process easily.
The fixture descendant that I'm using can change its own name via process.title but cannot on Windows. I've just tested this on Windows 10.
There are three solutions that comes to mind:
- create a binary fixture for Windows only that spawn a descendant and has a custom process name
- kill indiscriminately all processes called
node.exe - change the process title using some other tool, better if built-in in Windows (but I've found nothing so far and haven't searched enough)
|
|
||
| if (process.platform !== 'win32') { | ||
| // eslint-disable-next-line ava/test-ended | ||
| test.cb('kill all descendants tree by name', testKillDescendant, 'fkill-descen'); |
There was a problem hiding this comment.
| test.cb('kill all descendants tree by name', testKillDescendant, 'fkill-descen'); | |
| test.cb('kill all descendants tree by name', testKillDescendant, 'fkill-descendant'); |
?
There was a problem hiding this comment.
Sorry for not mentioning it before.
I've used the string "fkill-descen" because it's 12 chars, which is the theoretical maximum of chars I can change on process.title because of the limitation that have Linux and macOS as explained in the Node.js process.title docs.
If I'd got above the limit it might not match the process title and fail the test.
I'll have a look. I've already seen that issue but for some reasons, which I don't recall now, I've decided to use |
AFAIK macOS and Linux can use the same technique for killing a process tree and a single process via `process.kill`, as such their implementation have been merged into `defaultKill`; I've removed missingBinaryError, because it was used for checking if the commands `kill` or `killall` were available on the host, but now it just use `process.kill`. `options.tree` has been defaulted from the start because it's now used for both the kill methods implementations (Linux/macOS and Windows). The type of `input` in `handleKill` is now more important and the method have been streamlined; if the input is a: - process name (string), it gathers all the pids, similar to what `killAll` would do, and filter all the processes by it's name, excluding the `fkill` process itself; - process PID (number), it checks the existance of the process and proceed to kill it. As per the above changes the check for the existance of a process have been moved from the catch and duplicated accordingly to the type of the input.
… and won't kill itself
|
Before trying to fix the last test "don't kill |
|
How can this move forward? |
|
I'll try to think of something, but it's been so long I need some time to understand where I left this haha |
|
Closing as this is not moving forward. |
Fixes #21
Added
treeoption to default (linux) and macOS kill methods, like it has already been implemented for Windows.When
treeoption is enabled the process specified by the input, and its descendants, will be killed.To kill the whole tree it has been added and used the module
tree-kill.To handle a process name as input, which can match to multiple processes, it has been added the function
getPidsFromInput, which usesps-listto get all the pids by the proces name. This is needed becausetree-killneeds to know the PID of the parent process.Some fixtures have been added for test. The tests added check if the process and its descendant has been killed as well. A test also tries to kill a named process, to test
getPidsFromInput.