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

Amend RFC 517: Add material on std::process #579

Merged
merged 3 commits into from
Feb 11, 2015

Conversation

aturon
Copy link
Member

@aturon aturon commented Jan 13, 2015

The IO reform RFC is being split into several semi-independent pieces, posted as PRs like this one.

This RFC amendment discusses std::process.

Rendered

@abonander
Copy link

Can Child include a signal_interrupt() method that sends SIGINT for processes that can exit nicely with Ctrl-C? Or at least include other signal constants where the signal function is going that so that it doesn't require importing them from libc. 😄

It would be nice (if a bit verbose) if the stdin, stdout, stderr fields were Box<Writer + Send + Clone> because one of the boons of the current API is that the PipeStreams can be cloned and sent to another thread for reading, whereas the new API seems to complicate matters by requiring shared references or moves.

* `stdin`, `stdout` and `stderr` will be retained as public fields,
but their types will change to `Box<Reader+Send>` or
`Box<Writer+Send>` as appropriate. This effectively hides the internal
pipe infrastructure.
Copy link
Member

Choose a reason for hiding this comment

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

I think this was mentioned on the previous RFC, but it may be better to use concrete types here to allow addition of extension traits for platform-specific APIs (such as getting the file descriptor on unix).

@alexcrichton
Copy link
Member

@cybergeek94

Can Child include a signal_interrupt() method that sends SIGINT for processes that can exit nicely with Ctrl-C? Or at least include other signal constants where the signal function is going that so that it doesn't require importing them from libc. 😄

Indeed! The plan here is to move these unix-only functions into an extension trait inside of std::os. The module and/or trait could make it more convenient than importing from libc certainly.

It would be nice (if a bit verbose) if the stdin, stdout, stderr fields were Box<Writer + Send + Clone> because one of the boons of the current API is that the PipeStreams can be cloned and sent to another thread for reading, whereas the new API seems to complicate matters by requiring shared references or moves.

I think this came up on the original RFC, but I've recommended that we move to concrete types instead of trait objects to allow future extensions to be added to them in the future (such as cloning).

* Rename `cwd` to `current_dir`, take `AsPath`.
* Rename `spawn` to `run`
* Move `uid` and `gid` to an extension trait in `os::unix`
* Make `detached` take a `bool` (rather than always setting the
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if this had the similar API as Thread. i.e. keep spawn which spawns a detached process and add scoped which creates a bound/attached process that you can detach any time later.

Copy link
Member

Choose a reason for hiding this comment

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

This form of detached actually has a somewhat different semantic meaning than the detached for threads, which is good to point out! For threads you can simply choose at some point during their lifecycle to detach, but for processes there's 2 decisions to make:

  1. When spawning, the detached flag affects how the child process is spawned.
  2. When dropping a Child, whether or not to wait() for the child.

I think that the use case of spawning threads and processes is different enough to keep the APIs different, but we may want to explore various names.

@d3zd3z
Copy link

d3zd3z commented Jan 13, 2015

+1 on the std connectors inheriting by default for many invocation cases.

One problem I currently have (that looks unchanged) is that all of the fields of Command are private, and the construction methods are fairly limited. It works ok for code just building a simple command. However, for example, I want to have a method in my code that will possibly augment a command by running it with sudo. Currently there isn't any easy way of doing this, because the fields are private. Perhaps there could be a way of accessing or manipulating the command and argument data. This might also be solvable with the proposal being discussed elsewhere to allow outside impls to access private fields if desired.

command to detached mode).

The `stdin`, `stdout`, `stderr` methods will undergo a more
significant change. By default, the corresponding options we be
Copy link
Contributor

Choose a reason for hiding this comment

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

s/we/will/

pipe infrastructure.
* The `kill` method is dropped, and `id` and `signal` will move to `os::platform` extension traits.
* `signal_exit`, `signal_kill`, `wait`, and `forget` will all stay as they are.
* `wait_with_output` will take `&self`.
Copy link

Choose a reason for hiding this comment

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

do you mean &mut self?

@Stebalien
Copy link
Contributor

As far as I can tell, this proposal doesn't allow for piping directly from one Child to another. Is there any reason the the Stdio enum can't have a constructor that takes a PipeStream?

@alexcrichton
Copy link
Member

@Stebalien The outline of this RFC is largely geared towards categorizing today's implementation as well as ensuring that it has ample room to grow in the future. For example today piping from one child to another is difficult to do (requires mucking around with raw file descriptors).

That's not to say that this RFC is saying it can't be done, however. The modifications to the StdioContainer interface as well as using a concrete type for the input/output streams in Process should allow for future expansion to this form of API. You would, for example, request that stdout becomes a pipe, and then you would consume that pipe by passing it to another Command to use as its own stream (for example). These additions are all backwards compatible to make in the future.

@Stebalien
Copy link
Contributor

@alexcrichton got it. Thanks.

@aturon
Copy link
Member Author

aturon commented Feb 3, 2015

All: I've updated this RFC with a few minor fixes. The only substantial change is to not use trait objects for stdin and friends, but instead to use a newtyped reader/writer value to hide the implementation details.

As to reading data out of Command, that should be much more feasible now that we're moving to OsString.

Overall, there hasn't been a ton of feedback here, but I think that's largely because the current APIs are working well. This RFC will come up for a decision in the near future so if you have more feedback, please leave more comments!

@l0kod
Copy link

l0kod commented Feb 8, 2015

The detached() set a new session for the process but there is no way to change the controlling terminal (e.g. setsid -c). A simple approach would be to add a new function for this feature.

However, a more flexible approach would be to add a pre-execution closure to the Command. This could allow to replace uid(), gid() and detached() (and maybe more) with something like:

impl Command {
    fn prepare<'a, T>(&'a mut self, prepare: T) -> &'a mut Command
        where T: FnOnce() -> IoResult<()>, T: Send + 'a {}
}

@kjpgit
Copy link

kjpgit commented Feb 8, 2015

running arbitrary code after a fork is not thread safe.

On Sat, Feb 7, 2015 at 7:09 PM, Mickaël Salaün notifications@github.com
wrote:

The detached() set a new session for the process but there is no way to
change the controlling terminal (e.g. setsid -c). A simple approach would
be to add a new function for this feature.

However, a more flexible approach would to be able to add a
pre-execution closure to the Command. This could allow to replace uid(),
gid() and detached() (and maybe more) with something like:

impl Command {
fn prepare<'a, T>(&'a mut self, prepare: T) -> &'a mut Command
where T: FnOnce() -> IoResult<()>, T: Send + 'a {…}
}


Reply to this email directly or view it on GitHub
#579 (comment).

@l0kod
Copy link

l0kod commented Feb 10, 2015

Is there any way to execute some safe code after a fork? A pre-exec hook would be very handy.

@l0kod
Copy link

l0kod commented Feb 10, 2015

Well, I think a pre-exec hook would be unsafe anyway, so an unsafe fn prepare(…) should fit the purpose, isn't it?

@alexcrichton alexcrichton merged commit 2afff0e into rust-lang:master Feb 11, 2015
aturon added a commit to aturon/rust that referenced this pull request Feb 14, 2015
Per [RFC 579](rust-lang/rfcs#579), this commit
adds a new `std::process` module. This module is largely based on the
existing `std::old_io::process` module, but refactors the API to use
`OsStr` and other new standards set out by IO reform.

The existing module is not yet deprecated, to allow for the new API to
get a bit of testing before a mass migration to it.
bors added a commit to rust-lang/rust that referenced this pull request Feb 14, 2015
Per [RFC 579](rust-lang/rfcs#579), this commit
adds a new `std::process` module. This module is largely based on the
existing `std::old_io::process` module, but refactors the API to use
`OsStr` and other new standards set out by IO reform.

The existing module is not yet deprecated, to allow for the new API to
get a bit of testing before a mass migration to it.
@l0kod l0kod mentioned this pull request Mar 21, 2015
7 tasks
@Centril Centril added the A-process Proposals relating to spawning and communicating with processes. label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-process Proposals relating to spawning and communicating with processes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.