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

Tracking Issue for process_set_process_group #93857

Closed
4 tasks done
krallin opened this issue Feb 10, 2022 · 11 comments
Closed
4 tasks done

Tracking Issue for process_set_process_group #93857

krallin opened this issue Feb 10, 2022 · 11 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. O-fuchsia Operating system: Fuchsia T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@krallin
Copy link
Contributor

krallin commented Feb 10, 2022

Feature gate: #![feature(process_set_process_group)]

This is a tracking issue for: Add a process_group method to UNIX CommandExt

Add a process_group method to std::os::unix::process::CommandExt that
allows setting the process group id (i.e. calling setpgid) in the child, thus
enabling users to set process groups while leveraging the posix_spawn fast
path.

Public API

trait CommandExt {
    fn process_group(
        &mut self,
        pgroup: i32,
    ) -> &mut process::Command
}

Steps / History

Unresolved Questions

  • None yet.
@krallin krallin added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 10, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 11, 2022
…shtriplett

Add a `process_group` method to UNIX `CommandExt`

- Tracking issue: rust-lang#93857
- RFC: rust-lang/rfcs#3228

Add a `process_group` method to `std::os::unix::process::CommandExt` that
allows setting the process group id (i.e. calling `setpgid`) in the child, thus
enabling users to set process groups while leveraging the `posix_spawn` fast
path.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 11, 2022
…shtriplett

Add a `process_group` method to UNIX `CommandExt`

- Tracking issue: rust-lang#93857
- RFC: rust-lang/rfcs#3228

Add a `process_group` method to `std::os::unix::process::CommandExt` that
allows setting the process group id (i.e. calling `setpgid`) in the child, thus
enabling users to set process groups while leveraging the `posix_spawn` fast
path.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 1, 2022
…olnay

Add a `process_group` method to UNIX `CommandExt`

- Tracking issue: rust-lang#93857
- RFC: rust-lang/rfcs#3228

Add a `process_group` method to `std::os::unix::process::CommandExt` that
allows setting the process group id (i.e. calling `setpgid`) in the child, thus
enabling users to set process groups while leveraging the `posix_spawn` fast
path.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 1, 2022
…olnay

Add a `process_group` method to UNIX `CommandExt`

- Tracking issue: rust-lang#93857
- RFC: rust-lang/rfcs#3228

Add a `process_group` method to `std::os::unix::process::CommandExt` that
allows setting the process group id (i.e. calling `setpgid`) in the child, thus
enabling users to set process groups while leveraging the `posix_spawn` fast
path.
@dtolnay
Copy link
Member

dtolnay commented Mar 18, 2022

This may or may not need a Fuchsia implementation. See #93858 (comment).

@rustbot ping fuchsia

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2022

Hey friends of Fuchsia! This issue could use some guidance on how this should be
resolved/implemented on Fuchsia. Could one of you weigh in?

cc @tmandry

@rustbot rustbot added the O-fuchsia Operating system: Fuchsia label Mar 18, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 19, 2022
…olnay

Add a `process_group` method to UNIX `CommandExt`

- Tracking issue: rust-lang#93857
- RFC: rust-lang/rfcs#3228

Add a `process_group` method to `std::os::unix::process::CommandExt` that
allows setting the process group id (i.e. calling `setpgid`) in the child, thus
enabling users to set process groups while leveraging the `posix_spawn` fast
path.
@niklasf
Copy link
Contributor

niklasf commented Apr 27, 2022

Some updates:

@joshtriplett
Copy link
Member

T-libs-api, shall we stabilize process_group?

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jun 12, 2022

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 12, 2022
@BurntSushi
Copy link
Member

This LGTM, but can we improve the docs for process_group please? I think it should at least mention:

  • Why this routine might be useful. When does someone want to use process groups?
  • Why it's superior to setting the process group via pre_exec. (I don't think we want to be overly specific here, but a hint saying something like, "this may allow the implementation to use a faster path" would be nice.)

@joshtriplett
Copy link
Member

This LGTM, but can we improve the docs for process_group please? I think it should at least mention:

👍

* Why this routine might be useful. When does someone want to use process groups?

Some suggested documentation:

"Process groups determine which processes receive a signal when the user kills a process from its controlling terminal (e.g. hitting Ctrl-C to generate SIGTERM or Ctrl-\ to generate SIGQUIT). Shells and other programs that launch other programs use process groups to partition launched programs, such as foreground and background processes."

* Why it's superior to setting the process group via `pre_exec`. (I don't think we want to be overly specific here, but a hint saying something like, "this may allow the implementation to use a faster path" would be nice.)

As well as being safe, instead of the unsafe pre_exec.

@tmandry
Copy link
Member

tmandry commented Jun 25, 2022

This may or may not need a Fuchsia implementation. See #93858 (comment).

Fuchsia does have a concept of jobs which have the same or similar role as process groups, but doesn't implement the POSIX notion of a process group id that's a universal integer. Instead, jobs (like all kernel objects) are represented by random 32-bit handles that are issued to a process by the kernel.

It might be possible to shoehorn this into the POSIX interface by passing a handle as a pid_t, but it's not clear whether this is a good idea (and anyway, we might remove some or all of std::os::unix for Fuchsia as @niklasf mentioned). Definitely don't block on us.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 29, 2022
@rfcbot
Copy link

rfcbot commented Jun 29, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 29, 2022
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 9, 2022
@rfcbot
Copy link

rfcbot commented Jul 9, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jul 9, 2022
niklasf added a commit to niklasf/rust that referenced this issue Jul 9, 2022
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 17, 2022
…ss_group, r=joshtriplett

Document and stabilize process_set_process_group

Tracking issue: rust-lang#93857

FCP finished here: rust-lang#93857 (comment)
@JohnTitor
Copy link
Member

Stabilized by #99088, closing.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 8, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
… r=joshtriplett

Document and stabilize process_set_process_group

Tracking issue: rust-lang/rust#93857

FCP finished here: rust-lang/rust#93857 (comment)
progval added a commit to progval/tokio that referenced this issue Aug 23, 2023
Building on v1.63 fails with:

```
error[E0658]: use of unstable library feature 'process_set_process_group'
   --> ~/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.29.0/src/process/mod.rs:777:18
    |
777 |         self.std.process_group(pgroup);
    |                  ^^^^^^^^^^^^^
    |
    = note: see issue #93857 <rust-lang/rust#93857> for more information
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. O-fuchsia Operating system: Fuchsia T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants