8000 timeout: fails to kill child process when given as sh -c · Issue #3489 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

timeout: fails to kill child process when given as sh -c #3489

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.

Sign up for GitHub

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

Jump to bottom
Closed
jfinkels opened this issue May 6, 2022 · 4 comments
Closed

timeout: fails to kill child process when given as sh -c #3489

jfinkels opened this issue May 6, 2022 · 4 comments
Labels
U - timeout

Comments

@jfinkels
Copy link
Collaborator
jfinkels commented May 6, 2022 •
edited
Loading

Environment: Ubuntu 20.04.4, coreutils v8.30, sh links to dash

The uutils version of timeout fails to kill the child process when it is given as sh -c "...".

GNU timeout:

$ timeout 1 sh -c "yes > /dev/null"
$ ps | grep [y]es
# no output, meaning no running process, as expected

uutils timeout:

$ ./target/release/timeout 1 sh -c "yes > /dev/null"
$ ps | grep [y]es
2458257 pts/1    00:00:02 yes

However, uutils timeout does correctly kill the child process when it is given directly:

$ ./target/release/timeout 1 yes > /dev/null
$ ps | grep [y]es
# no output, meaning no running process, as expected

This causes an orphan process in the GNU test case tests/split/filter.sh.
@353fc443 353fc443 mentioned this issue May 6, 2022
Closed
@fritzrehde
Copy link

I cannot replicate your observations, I tried the three examples you provided and found that all kill the child process correctly.

@tertsdiepraam
Copy link
Member
tertsdiepraam commented Sep 4, 2023 •
edited
Loading

Interesting, maybe this fixed it? #4315

Also, thanks for checking and following up on all these issues!

@fritzrehde
Copy link

Yep, I just went through the code and found that PR/commit too. It looks good to me as well.
However, I am looking at the code that tests this fix right now:

coreutils/tests/by-util/test_timeout.rs

Lines 147 to 160 in 862a2df

fn test_kill_subprocess() {
new_ucmd!()
.args(&[
// Make sure the CI can spawn the subprocess.
"10",
"sh",
"-c",
"sh -c \"trap 'echo xyz' TERM; sleep 30\"",
])
.fails()
.code_is(124)
.stdout_contains("xyz")
.stderr_contains("Terminated");
}

and, when running locally with cargo test --features timeout test_kill_subprocess, the test fails, because the stderr does not contain Terminated. I will create a PR that fixes this and adds some documentation (since I didn't immediately understand what the test was doing and why).

@fritzrehde fritzrehde mentioned this issue Sep 4, 2023
Closed
@jfinkels
Copy link
Collaborator Author

Nice, I'm glad this was fixed! I'll close the issue then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
U - timeout
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jfinkels @tertsdiepraam @fritzrehde
0