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 1 commit 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.

try-job: dist-various*
try-job: test-various*

@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.

@ChrisDenton ChrisDenton added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2025
@Qelxiros
Copy link
Contributor Author

Qelxiros commented Jun 6, 2025

@tgross35 could you add an unresolved question to the tracking issue regarding & vs &mut? I want to make sure it doesn't get forgotten before stabilization.

@tgross35
Copy link
Contributor

tgross35 commented Jun 6, 2025

@tgross35 could you add an unresolved question to the tracking issue regarding & vs &mut? I want to make sure it doesn't get forgotten before stabilization.

Added.

I think this implementation looks fine, just needs a squash (I'll let Chris do the final review)

@Qelxiros
Copy link
Contributor Author

Qelxiros commented Jun 7, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 7, 2025
@Qelxiros Qelxiros force-pushed the 141975-unix_send_signal branch from 149dd21 to 269ee72 Compare June 12, 2025 03:02
@ChrisDenton
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 15, 2025

📌 Commit 269ee72 has been approved by ChrisDenton

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 15, 2025
tgross35 added a commit to tgross35/rust that referenced this pull request Jun 16, 2025
…r=ChrisDenton

Implement send_signal for unix child processes

Tracking issue: rust-lang#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`.
bors added a commit that referenced this pull request Jun 16, 2025
Rollup of 7 pull requests

Successful merges:

 - #138237 (Get rid of `EscapeDebugInner`.)
 - #140809 (Reduce special casing for the panic runtime)
 - #141990 (Implement send_signal for unix child processes)
 - #142082 (Refactor `rustc_attr_data_structures` documentation)
 - #142125 (Stabilize "file_lock" feature)
 - #142528 (clarify `rustc_do_not_const_check` comment)
 - #142530 (use `if let` guards where possible)

r? `@ghost`
`@rustbot` modify labels: rollup
@tgross35
Copy link
Contributor

tgross35 commented Jun 16, 2025

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 16, 2025
@@ -152,6 +152,11 @@ impl Process {
Ok(())
}

pub fn send_signal(&mut self) -> io::Result<()> {
// Fuchsia doesn't have a direct equivalent for signals
unimplemented!()
Copy link
Contributor

Choose a reason for hiding this comment

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

Cc target maintainers @erickt @Nashenas88 just to double check there isn't something that makes sense here (nonblocking for this PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

Fuchsia and unsupported need a _signal: i32 argument to match the other signatures

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, fuchsia does not support signals. Thanks for checking!

@tgross35

This comment was marked as outdated.

@rust-bors

This comment was marked as outdated.

rust-bors bot added a commit that referenced this pull request Jun 17, 2025
Implement send_signal for unix child processes

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`.

try-job: `dist-various*`
try-job: `test-various*`
@rust-bors

This comment was marked as outdated.

@Qelxiros Qelxiros force-pushed the 141975-unix_send_signal branch from 8396d56 to 9f255e9 Compare June 18, 2025 00:22
@tgross35

This comment was marked as outdated.

@rust-bors

This comment was marked as outdated.

rust-bors bot added a commit that referenced this pull request Jun 18, 2025
Implement send_signal for unix child processes

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`.

try-job: `dist-various*`
try-job: `test-various*`
@rust-bors

This comment was marked as outdated.

@Qelxiros Qelxiros force-pushed the 141975-unix_send_signal branch from 9f255e9 to ec665bf Compare June 18, 2025 02:35
@tgross35
Copy link
Contributor

@bors2 try

@rust-bors
Copy link

rust-bors bot commented Jun 18, 2025

⌛ Trying commit ec665bf with merge 1a4108f

To cancel the try build, run the command @bors2 try cancel.

rust-bors bot added a commit that referenced this pull request Jun 18, 2025
Implement send_signal for unix child processes

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`.

try-job: `dist-various*`
try-job: `test-various*`
@tgross35
Copy link
Contributor

So you can run yourself if needed:
@bors2 delegate=try

@rust-bors
Copy link

rust-bors bot commented Jun 18, 2025

@Qelxiros can now perform try builds on this pull request

@rust-bors
Copy link

rust-bors bot commented Jun 18, 2025

☀️ Try build successful (CI)
Build commit: 1a4108f (1a4108fc06b9150985b49010e05c520f8abe3c00, parent: 27eb2690f4d78f0f41eaa7193a06cd49d74b2eb0)

@tgross35
Copy link
Contributor

@Qelxiros mind squashing?

@Qelxiros Qelxiros force-pushed the 141975-unix_send_signal branch from ec665bf to b391494 Compare June 18, 2025 04:39
@Qelxiros
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 18, 2025
@tgross35
Copy link
Contributor

Thanks!

@bors r=ChrisDenton,tgross35

@bors
Copy link
Collaborator

bors commented Jun 18, 2025

📌 Commit b391494 has been approved by ChrisDenton,tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2025
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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants