Skip to content
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

Killing cargo test doesn't terminate the child process #14929

Open
fredrik-jansson-se opened this issue Dec 12, 2024 · 5 comments
Open

Killing cargo test doesn't terminate the child process #14929

fredrik-jansson-se opened this issue Dec 12, 2024 · 5 comments
Labels
C-bug Category: bug Command-test S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@fredrik-jansson-se
Copy link

fredrik-jansson-se commented Dec 12, 2024

Problem

I'm using bacon to run tests and when tests are restarted, bacon terminates the child (cargo test) process using SIGKILL. The problem is if the test doesn't terminate, the child process is not terminated.

NOTE, this is not bacon related but can be reproduced using cargo only.

#[test]
fn test() {
    loop {
        std::thread::sleep_ms(10_0000);
    }
}

I can reproduce this on both MacOS and Linux.

Steps

  1. Create a main.rs with a test that hangs, see above
  2. pkill -9 cargo
  3. ps aux | grep
  4. You'll find that the child process is still running (/home/frja/bacon-zombie/target/debug/deps/bacon_zombie-ab96b146d87eb3ef)

Possible Solution(s)

Not sure if it's possible to link the spawned child process... process groups maybe?

Notes

No response

Version

cargo 1.83.0 (5ffbef321 2024-10-29)
release: 1.83.0
commit-hash: 5ffbef3211a8c378857905775a15c5b32a174d3b
commit-date: 2024-10-29
host: x86_64-unknown-linux-gnu
libgit2: 1.8.1 (sys:0.19.0 vendored)
libcurl: 8.9.0-DEV (sys:0.4.74+curl-8.9.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Ubuntu 24.4.0 (noble) [64-bit]



cargo 1.83.0 (5ffbef321 2024-10-29)
release: 1.83.0
commit-hash: 5ffbef3211a8c378857905775a15c5b32a174d3b
commit-date: 2024-10-29
host: aarch64-apple-darwin
libgit2: 1.8.1 (sys:0.19.0 vendored)
libcurl: 8.7.1 (sys:0.4.74+curl-8.9.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Mac OS 15.1.1 [64-bit]
@fredrik-jansson-se
Copy link
Author

Hopefully a clearer example on how reproduce:

$ cargo test &
[1] 2155724
     Running unittests src/main.rs (target/debug/deps/bacon_zombie-ab96b146d87eb3ef)

running 1 test

$ ps
    PID TTY          TIME CMD
2152410 pts/6    00:00:00 zsh
2155724 pts/6    00:00:00 cargo
2155742 pts/6    00:00:00 bacon_zombie-ab
2155760 pts/6    00:00:00 ps

$ pkill -9 cargo
[1]  + 2155724 killed     cargo test

$ ps
    PID TTY          TIME CMD
2152410 pts/6    00:00:00 zsh
2155742 pts/6    00:00:00 bacon_zombie-ab
2155799 pts/6    00:00:00 ps

@fredrik-jansson-se
Copy link
Author

And finally (?), a huge thanks to everyone on the cargo team! What an amazing job you do!

@weihanglo
Copy link
Member

weihanglo commented Dec 17, 2024

If you run ps -opid,pgid,cmd, it'll show they are under the same process group already. pgrep -g <PGID> should also show that. So pkill -g <PGID> will do the trick for you. For kill command you put an - before PGID so it sends the signal to all processes in the process group, such as kill -9 -123.

From my understanding, on Linux killing parents doesn't automatically kill their children. Those orphan will just link to init process or something else. Not sure if we want cargo to be opinionated on process signal handling. To myself I would leave it to parent process of cargo.

Any reason the command invoking cargo can't handle this by itself?

@weihanglo weihanglo added S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. and removed S-triage Status: This issue is waiting on initial triage. labels Dec 17, 2024
@fredrik-jansson-se
Copy link
Author

I'm truly not an expert on the topic, but I guess the reason is that the code invoking cargo looks something like this:

fn main() {
    let mut cmd = std::process::Command::new("cargo")
        .args(["test"])
        .spawn()
        .expect("Launch cargo test");

    std::thread::sleep(std::time::Duration::from_secs(10));

    println!("Stopping test");
    cmd.kill().expect("kill test");
    cmd.wait().expect("waiting");
}

#[test]
fn test() {
    loop {
        std::thread::sleep(std::time::Duration::from_secs(10));
    }
}

Not sure how the above code could handle this better.

Probably obvious, but the above program exhibits the the problem:

$ cargo r
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/cargo-zombie`
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.00s
     Running unittests src/main.rs (target/debug/deps/cargo_zombie-75c5c9cb344a32d5)

running 1 test
Stopping test

$ ps aux | grep cargo_zombie
...
frja             66080   0.0  0.0 410605184   2032 s018  S     8:39AM   0:00.00 /Users/frja/dev/rust/tmp/cargo-zombie/target/debug/deps/cargo_zombie-75c5c9cb344a32d5

@weihanglo
Copy link
Member

Command::kill is equivalent to sending KILL on Unix. I believe it sends PID not -PID so only a specific process receives it, just like aformentioned kill(1).

Some alternative testing frameworks like nextest have their own specific execution model, so they chose to handle signals all by themselves. See https://nexte.st/docs/design/architecture/signal-handling/.

It is generally good if cargo test, as a test executor, manages signals for its child processes, so wrapper can simple kill. However, by just reading the design doc of nextest, technical details are way more complicated. I may wait for the testing-devex team to share their opinions in overall directions.

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix and removed S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. labels Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug Command-test S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
Status: No status
Development

No branches or pull requests

3 participants