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.

Sign up for GitHub

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
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
860f85f
Amend RFC 517: Add material on std::process
aturon Jan 13, 2015
6762d64
stdin etc are newtypes; with_output_timeout really wants self
aturon Feb 3, 2015
2afff0e
Fix typos
aturon Feb 3, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 60 additions & 2 deletions text/0517-io-os-reform.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ follow-up PRs against this RFC.
* [std::env]
* [std::fs] (stub)
* [std::net] (stub)
* [std::process] (stub)
* [std::process]
* [Command]
* [Child]
* [std::os]
* [Odds and ends]
* [The io prelude]
Expand Down Expand Up @@ -1239,7 +1241,63 @@ This brings the constants into line with our naming conventions elsewhere.
### `std::process`
[std::process]: #stdprocess

> To be added in a follow-up PR.
Currently `std::io::process` is used only for spawning new
processes. The re-envisioned `std::process` will ultimately support
inspecting currently-running processes, although this RFC does not
propose any immediate support for doing so -- it merely future-proofs
the module.

#### `Command`
[Command]: #command

The `Command` type is a builder API for processes, and is largely in
good shape, modulo a few tweaks:

* Replace `ToCStr` bounds with `AsOsStr`.
* Replace `env_set_all` with `env_clear`
* 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.

command to detached mode).

The `stdin`, `stdout`, `stderr` methods will undergo a more
significant change. By default, the corresponding options will be
considered "unset", the interpretation of which depends on how the
process is launched:

* For `run` or `status`, these will inherit from the current process by default.
* For `output`, these will capture to new readers/writers by default.

The `StdioContainer` type will be renamed to `Stdio`, and will not be
exposed directly as an enum (to enable growth and change over time).
It will provide a `Capture` constructor for capturing input or output,
an `Inherit` constructor (which just means to use the current IO
object -- it does not take an argument), and a `Null` constructor. The
equivalent of today's `InheritFd` will be added at a later point.

#### `Child`
[Child]: #child

We propose renaming `Process` to `Child` so that we can add a
more general notion of non-child `Process` later on (every
`Child` will be able to give you a `Process`).

* `stdin`, `stdout` and `stderr` will be retained as public fields,
but their types will change to newtyped readers and writers to hide 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).

* 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.
Copy link
Member

Choose a reason for hiding this comment

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

Taking another look at the windows implementation, it looks like the only common denominator is signal_kill as both exit/kill are canonicalized to TerminateProcess.

The signal_exit function may wish to live on a unix extension trait for now, and we can perhaps add a windows implementation at a future date.

Copy link

Choose a reason for hiding this comment

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

Since Windows doesn't really have signals, I would like signal_kill renamed to kill. Seeing any reference to signals on Windows seems foreign to me.

Copy link
Member

Choose a reason for hiding this comment

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

signal_kill renamed to kill

+1 for that another bikeshed colour.

Copy link

Choose a reason for hiding this comment

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

@alexcrichton It's fine to map SIGKILL to TerminateProcess(), but the common strategy to port SIGINT/SIGBREAK to Windows seems to be to use Win32 API GenerateConsoleCtrlEvent(). My understanding is that cmd.exe calls this API when we hit Ctrl-C on it. What's called "console event" in Win32 API also supports installation of arbitrary "handlers." The drawback is it is restricted to processes sharing the same "console."

See also:

Copy link
Member

Choose a reason for hiding this comment

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

Oh nice, thanks for the pointers @nodakai! The restriction about processes sharing the same console is a little worrying, but I think I'd be fine with providing both variants with this semi-close equivalent.

I'd also be fine tweaking the names, @jminer has a good point that "signal" is inherently unix-biased.

* `set_timeout` will be changed to use the `with_deadline` infrastructure.

There are also a few other related changes to the module:

* Rename `ProcessOutput` to `Output`
* Rename `ProcessExit` to `ExitStatus`, and hide its
representation. Remove `matches_exit_status`, and add a `status`
method yielding an `Option<i32>`
* Remove `MustDieSignal`, `PleaseExitSignal`.
* Remove `EnvMap` (which should never have been exposed).

### `std::os`
[std::os]: #stdos
Expand Down