Skip to content

Implement send_signal for unix child processes #141990

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Qelxiros
Copy link
Contributor

@Qelxiros Qelxiros commented Jun 3, 2025

Tracking issue: #141975

There are two main differences between my implementation and the Public API section of the tracking issue. First, send_signal requires a mutable reference, like Child::kill. Second, ChildExt has Sealed as a supertrait, bringing it more in line with other extension traits like CommandExt.

@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2025

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 3, 2025
@ChrisDenton
Copy link
Member

cc @tgross35 as this modifies the proposed API slightly

- fn send_signal(&self, signal: i32) -> Result<()>;
+ fn send_signal(&mut self, signal: i32) -> Result<()>;

@tgross35
Copy link
Contributor

tgross35 commented Jun 3, 2025

I didn't put too much thought into what I wrote there, so it's fine if it needs to change. That being said, why does this need to be &mut? kill should be MT-Safe https://www.gnu.org/software/libc/manual/2.28/html_node/Signaling-Another-Process.html

@Qelxiros could you add documentation and examples here?

@Qelxiros
Copy link
Contributor Author

Qelxiros commented Jun 4, 2025

You're right that it doesn't need to be mutable, so I changed it back. I do wonder why e.g. Child::kill takes a mutable reference. Is it required on windows? If so, is it worth adding a kill-specific method to the unix ChildExt?

@ChrisDenton
Copy link
Member

ChrisDenton commented Jun 4, 2025

No it's not to do with Windows. It seems to have been a deliberate decision in the early days of rust, though I can't find any record of the thinking behind it. It was implemented in #22119 and all the implementations use &self behind the scenes, only the public interface used &mut self.

/// # Errors
///
/// This function will return an error if the signal is invalid. The integer values associated
/// with signals are implemenation-specific, so the use of [`libc`] is encouraged.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// with signals are implemenation-specific, so the use of [`libc`] is encouraged.
/// with signals are implemenation-specific, so the use of `libc` is encouraged.

libc is not a public dependency so linking won't work. Also it might be worth saying something more generic. There are other crates that provide posix bindings (e.g. for safety or other reasons). They might use libc internally but the user doesn't necessarily need to know that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants