For some executables, cntr-c will leave zombie processes

[sagiegurari]

This is in to continue #355 since original issue was resolved, but additional flow reported by @mogenson was not and requires different handling.
There is a hacky workaround to use script executable for it in the meantime.

repro can be seen in https://github.com/mogenson/cargo-make-ctrl-c.git

[MartinKavik]

Hi,
I think I have a similar problem with Ctrl+C.
A cargo-make task is stopped by Ctrl+C but the process started by the associated command is shutting down and still writing to the terminal. The result is this:

The task is just a cargo run command; the executed app uses tokio::signal::ctrl_c to implement custom graceful shut down:

# ...
[tasks.mzoon]
description = "Run MZoon"
command = "cargo"
args = ["run", "--manifest-path", "../../crates/mzoon/Cargo.toml", "${@}"]

The first idea how to resolve it - add the forward_signals or ignore_signals property with a bool or enum/array value:

[tasks.mzoon]
ignore_signals = ["all"]   # <----
command = "cargo"
args = ["run", "--manifest-path", "../../crates/mzoon/Cargo.toml", "${@}"]

Then cargo-make would wait for the task to finish even after the user pressed Ctrl+C.

[sagiegurari]

@MartinKavik thanks for the feedback.
The idea is interesting but i'm worried about use cases like the one that originally opened this issue which is a sub process that runs forever (such as a server) until you break it.

is there any signal you are getting that parent process died in your sub process? i guess the ctrlc you are not getting but i wonder whats there that prevents the process from being killed when the parent dies as well and how i can force kill those.

[MartinKavik]

I'm not sure if it'll answer your question, but:

I've made a simple experiment - added ctrlc = "3.2.1" to cargo-make's Cargo.toml and modified run_command_get_output:

/// Runs the requested command and return its output.
pub(crate) fn run_command_get_output(
    command_string: &str,
    args: &Option<Vec<String>>,
    capture_output: bool,
) -> io::Result<Output> {

    // --- NEW ---
    ctrlc::set_handler(move || {
        info!("Ctrl-C handled!");
    }).expect("Error setting Ctrl-C handler");
    // --- // ---

    debug!("Execute Command: {}", &command_string);
    // ...

Then I executed cargo run --manifest-path "../../../cargo-make/Cargo.toml" --bin "makers" -- mzoon start to run the modified cargo-make version and pressed Ctrl+C after a while and the result is:

(compare with the image in the previous comment #374 (comment))

As you can see, all running parts - cargo-make task, mzoon (Rust CLI app) and the server (based on Actix) - gracefully finished.

So my observed understanding is that all processes and sub-processes receive Ctrl+C signal from OS and then individual signal handlers are triggered or the process is killed when there is only a default handler.
In my case, on Ctrl+C signal:

1. Actix server handles the signal and starts its graceful shutdown.
2. mzoon was waiting for the signal (tokio::signal::ctrl_c().await?;) and now it's waiting for the Actix server (server.wait().await, where server is tokio::process:Child).
3. cargo-make handles/ignores the signal and it's still waiting for mzoon process to finish (command.output();)
4. When Actix finishes then mzoon finishes and then makers (cargo-make task) finishes gracefully without any killing or additional Ctrl+C signal sending among the processes.

I don't know how it works on Windows but you can still press Ctrl+\ on Linux to stop the process and then the result is the same as the one without the Ctr+C custom handler.

I think an interesting (and maybe a bit standard (?)) approach is to try to gracefully shutdown all involved processes on the first Ctrl+C press and kill the script/command on the second press. This way there won't be zombie processes because the cargo-make task would kill them before gracefully ending itself.

So I think/hope the "double Ctrl+C signal" described above would solve both problems with zombie/finishing processes. Perhaps there are some disadvantages but I can't see them now.

[sagiegurari]

@MartinKavik very interesting. maybe if we modify what you did to

1. print shutting down...
2. wait for a half sec or so, so child process get the signal and do their thing
3. shutdown cargo make process
   that might allow us to remove the need of double ctrlc + allow child process to handle the signal.
   the only question, if step 3 comes before the child processes finished to terminate, would they still hang?

if it does solve it, I would really appreciate a PR :)

[MartinKavik]

the only question, if step 3 comes before the child processes finished to terminate, would they still hang?

A cargo make task has to explicitly kill its children - without killing we just add a delay to the current cargo make behavior I think.
Timeouts are always tricky. I can imagine shutting down long-running processes with things like embedded temporary databases (for instance blockchain dev nodes/validators or MoonZoon apps in my case in the future) will take more than a second to finish because of file system operations.

I would start with double ctrl+c + print "shutting down..." message as you suggested and eventually add opt-in timeout in the future if needed. What do you think?

[sagiegurari]

I like the opt-in idea.
I wonder if we can 'flag' specific tasks to enable this behavior only for them somehow.
because the issue doesn't happen all of the time, only to specific child processes.
So maybe, we could add some 'execution hints' struct to the toml. something that we can extend later on...

[tasks.mytask]
command = "problem"
execution_hints = { "pass_break_signal" = true }

not nice... but it could give a workaround to the issue which is pretty rare without breaking lots of other stuff.

[sagiegurari]

@MartinKavik I looked at that crate.

1. you can't remove handler
2. you can't replace a handler
3. you can't check if handler is defined.
   it is a bit limited for some reason.

[sagiegurari]

This is now officially released

[rino0601]

Hi @sagiegurari I just found that execution_hints = { "pass_break_signal" = true } is what I looking for.

But it doesn't mention on README.md and https://sagiegurari.github.io/cargo-make/ and whatever called this:

The comments above is the only one I can find that mention 'execution hints'

I like the opt-in idea. I wonder if we can 'flag' specific tasks to enable this behavior only for them somehow. because the issue doesn't happen all of the time, only to specific child processes. So maybe, we could add some 'execution hints' struct to the toml. something that we can extend later on...

[tasks.mytask]
command = "problem"
execution_hints = { "pass_break_signal" = true }

not nice... but it could give a workaround to the issue which is pretty rare without breaking lots of other stuff.

It would be great included in the user documentation.

---

Plus, I found that log said execution_hints is unknown key:

(moneycircuit) ➜  moneycircuit git:(main) ✗ cargo make dev      
[cargo-make] INFO - cargo make 0.35.16
[cargo-make] WARN - Found unknown key: tasks.?.dev.execution_hints in file: ./Makefile.toml
[cargo-make] INFO - Build File: Makefile.toml

but it actually works. the existance of execution_hints make a defferent behavior. how could it be?

---

To help you understand here is my Makefile.toml:

[config]
default_to_workspace = false

[tasks.techdocs]
description = "Backstage 의 techdocs 로 docs/ 아래 md 파일을 렌더링 합니다."
category = "Documentation"
command = "npx"
args = ["@techdocs/cli", "serve"]

[tasks.svelte-dev]
cwd = "moneycircuit-svelte"
command = "npm"
args = ["run", "dev"]

[tasks.rust-dev]
command = "cargo"
args = ["run", "-p", "moneycircuit"]

[tasks.dev]
description = "spwan all development relative process."
# https://github.com/sagiegurari/cargo-make/issues/374#issuecomment-1103619644
execution_hints = { "pass_break_signal" = true }  # if without it, Ctrl+C left over child process. it cause a error on next run.
run_task = { name = ["techdocs", "rust-dev", "svelte-dev"], parallel = true }

[sagiegurari]

@rino0601 execution hints are not documented because it doesn't exist :)
instead there are 'unstable features'
see
https://github.com/sagiegurari/cargo-make#unstable-features