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

Clarify Child::kill behavior #7162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

stepancheg
Copy link
Contributor

@stepancheg stepancheg commented Feb 16, 2025

Child::kill       == std Child::kill + Child::wait
Child::start_kill == std Child::kill

As a side note, kill that also waits, is convenient, but also
dangerous, because wait may hang in certain cases (NFS, ptrace,
etc). And different behavior between std and tokio may leads to
bugs when code is mechanically converted from one to another. Better
name name of this function might be kill_and_wait.

CC @maminrayej

@maminrayej maminrayej added T-docs Topic: documentation A-tokio Area: The main tokio crate M-process Module: tokio/process labels Feb 16, 2025
@@ -1164,7 +1164,11 @@ impl Child {

/// Forces the child to exit.
///
/// This is equivalent to sending a `SIGKILL` on unix platforms.
/// This is equivalent to sending a `SIGKILL` on unix platforms followed by `wait`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make wait a link for easier doc navigation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added links.

Comment on lines 1169 to 1171
/// Note: behavior of this function is different from std version of
/// [`Child::kill`](std::process::Child::kill): std version does `wait`.
/// Tokio equivalent of std `Child::kill` is [`start_kill`](Child::start_kill).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think the phrasing could be a bit simplified. Like:

Note: The equivalent of `Child::kill` in the standard library is [`start_kill`](Child::start_kill).

Copy link
Contributor Author

@stepancheg stepancheg Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot understabd your version (what side is what); rephrased.

```
Child::kill       == std Child::kill + Child::wait
Child::start_kill == std Child::kill
```

As a side note, kill that also waits, is convenient, but also
dangerous, because `wait` may hang in certain cases (NFS, ptrace,
etc).  And different behavior between std and tokio may leads to
bugs when code is mechanically converted from one to another.  Better
name name of this function might be `kill_and_wait`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-process Module: tokio/process T-docs Topic: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants