-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC for redirecting stdio of child processes to open file handles #1055
Closed
Closed
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
dc55144
RFC for redirecting stdio of child processes to open file handles
ipetkov c854d08
Improve and rework design
ipetkov 2d67ee4
Make sure that Stdio stores an Arc<File>, not Rc<File>
ipetkov 4e19aef
Rework design and simplify design again
ipetkov 2f0e002
Update location of `StdioExt`
ipetkov 8b8a468
Correct example usage
ipetkov 4ac365e
AnonPipes no longer use file descriptors on Windows
ipetkov 70e4043
Update proposal
ipetkov 113bd5a
Add method signatures for each high level API alternative
ipetkov 690d99b
Update the motivation behind the RFC
ipetkov a8e1c8b
Pick a favorite design approach
ipetkov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
- Feature Name: process-stdio-redirection | ||
- Start Date: 2015-04-10 | ||
- RFC PR: | ||
- Rust Issue: | ||
|
||
# Summary | ||
|
||
Update the standard library with a cross-platform high level API for redirecting | ||
stdio of child processes to any opened file handle or equivalent. | ||
|
||
# Motivation | ||
|
||
The current API in `std::process` allows the usage of raw file descriptors or | ||
HANDLEs for child process stdio redirection by leveraging the `FromRaw{Fd, | ||
Handle}` and `AsRaw{Fd, Handle}` trait implementations on `Stdio` and | ||
`ChildStd{In, Out, Err}`, respectively. | ||
|
||
Unfortunately, since the actual methods pertaining to `FromRaw*` and `AsRaw*` | ||
are OS specific, their usage requires either constant `cfg` checks or OS | ||
specific code duplication. Moreover, the conversion to `Stdio` is `unsafe` and | ||
requires the caller to ensure the OS handle remains valid until spawning the | ||
child. In the event that a caller wishes to give a child exclusive ownership of | ||
an OS handle, they must still go through the headache of manually keeping the | ||
handle alive and valid. | ||
|
||
Developing a high level cross-platform API will make stdio redirection more | ||
ergonomic and reduce code noise. | ||
|
||
# Detailed design | ||
|
||
The de facto method for adding system specific extensions to the standard | ||
library has been to define an extension trait--following this approach a | ||
`StdioExt` trait should be defined under `std::os::$platform::process` to | ||
provide the redirection functionality. Unlike other system specific extensions, | ||
however, the methods of this trait should match lexically, differing only in the | ||
`AsRaw*` type they accept, such that rebuilding the same source on a different | ||
platform will only require the import of the OS specific trait rather than | ||
changing method invodations as well. | ||
|
||
This trait should define two methods which accept the appropriate `AsRaw*` | ||
implementor and return an `Stdio`: | ||
* One which (safely) takes ownership of the raw handle or its wrapper. The | ||
wrapper should be boxed and stored by the `Stdio` wrapper so its destructor | ||
can run when it goes out of scope. | ||
* Another method which simply extracts the raw handle without taking ownership: | ||
this method will essentially be a cross-platform abstraction over using | ||
`FromRaw*` on `Stdio`, thus making this method `unsafe` as well. | ||
|
||
```rust | ||
pub trait StdioExt { | ||
// Take ownership of the handle | ||
fn redirect<T: AsRaw*>(t: T) -> Stdio; | ||
// Unsafely borrow the handle, letting caller ensure it is valid | ||
unsafe fn redirect_by_ref<T: AsRaw*>(t: &T) -> Stdio; | ||
} | ||
``` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that you could also do something like blending strategies 1/2: pub trait StdioExt {
pub fn redirect<T: AsRawFd>(t: T) -> Stdio;
}
impl StdioExt for Stdio { ... } |
||
|
||
Example API usage with minimal `cfg` checks and safe methods: | ||
|
||
```rust | ||
#[cfg(unix)] use std::os::unix::process::StdioExt; | ||
#[cfg(windows)] use std::os::windows::process::StdioExt; | ||
|
||
// Equivalent of `foo | bar` | ||
let foo = Command::new("foo").stdout(Stdio::piped()).spawn().unwrap(); | ||
let out = foo.stdout.take().unwrap(); | ||
let bar = Command::new("bar").stdin(Stdio::redirect(out)).spawn().unwrap(); | ||
``` | ||
|
||
# Drawbacks | ||
|
||
Without using a high level API callers will be forced to use verbose and | ||
`unsafe` code more than they should or could get away with. Even with using a | ||
high level API there will be `unsafe`ty present due to stability lock on | ||
`Command` and `Stdio` (i.e. we cannot simply store references to the handles | ||
ensuring they remain valid). However, `Command`s are are usually spawned | ||
immediately after building during which time open file descriptors/HANDLEs are | ||
still valid. | ||
|
||
# Alternatives | ||
|
||
An alternative approach would be to expose `redirect` methods directly on | ||
`Stdio`. This design, however, blurs the distinction of platform specific | ||
details (e.g. a file and socket are both file descriptors on Unix, but not | ||
necessarily HANDLEs on Windows) and may cause some confusion and give rise to | ||
platform specific bugs. | ||
|
||
```rust | ||
impl Stdio { | ||
pub fn piped() -> Stdio; | ||
pub fn inherit() -> Stdio; | ||
pub fn null() -> Stdio; | ||
|
||
// Take ownership of the handle | ||
#[cfg(unix)] pub fn redirect<T: AsRawFd>(t: T) -> Stdio; | ||
#[cfg(windows)] pub fn redirect<T: AsRawHandle>(t: T) -> Stdio; | ||
|
||
// Unsafely borrow the handle, letting caller ensure it is valid | ||
#[cfg(unix)] pub unsafe fn redirect_by_ref<T: AsRawFd>(t: T) -> Stdio ; | ||
#[cfg(windows)] pub unsafe fn redirect_by_ref<T: AsRawHandle>(t: T) -> Stdio; | ||
} | ||
``` | ||
|
||
# Unresolved questions | ||
|
||
None at the moment. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API is actually a little subtle in this regard, but the contract of
FromRawFd
is that it takes ownership of the file descriptor (or handle) in question, so the unsafety is based on ownership, not on the lifetime of the resource itself. Specifically, the various primitives in the standard library provide the guarantee that they are the sole owner of the fd/handle in question, and this being safe would otherwise be a violation of that.Also, if we assume that a
IntoRawFd
trait exists, using the raw underlying interfaces may not be so bad as you'd just have two functions (unix/windows) doingStdio::from_raw_fd(file.into_raw_fd())
. I suspect that adding traits likeIntoRawFd
are somewhat inevitable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be more concrete, one concern I have about this is that this RFC is based on an ergonomic concern which will be alleviated soon in the future. The alternative (
IntoRawFd
) I don't think is unergonomic enough to warrant a specialized function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcrichton
Ah, I had overlooked this...
However, doesn't this require that the caller
mem::forget
the wrapper over the descriptor/HANDLE or at least manuallydup
it (and perform error checking)? Because the contract ofAsRawFd
does not transfer ownership and then bothStdio
and the original wrapper will both try to close the handle.I also agree, however, I feel like something like
IntoRawFd
could be very useful now, because being forced tomem::forget
ordup
handles (i.e. when using the current state of the API) is very unergonomic to force on userland, especially when the standard library does such a good job from abstracting much of that away (and the reason why theCommand
interface is so handy to have).I could definitely live with a compromise like
IntoRawFd
, but a specialized function could avoid accidental use ofAsRawFd
instead ofIntoRawFd
and prevent someone from shooting their foot (since both traits would produce a fd whichFromRawFd
will happily accept).Assuming the availability of an
IntoRawFd
trait the currently proposed API could be tweaked to use it more appropriately:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is correct, yes.
I think one part of this that I'm uneasy about is how
Stdio::redirect(t)
is the same asStdio::from_raw_fd(t.into_raw_fd())
. Along those lines it seems redundant to have theredirect
method, and then only adding an unsaferedirect_by_ref
method also seems somewhat out of place because it's relatively the same thing asStdio::from_raw_fd(f.as_raw_fd())
+ amem::forget
somewhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a point there; I suppose that minus the cross-platform benefits the RFC's design does not offer any further benefits than something like
IntoRawFd
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, in that case it may be good motivation to push harder on these traits perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so. Do you think this RFC would be an appropriate place to discuss them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would serve as good motivation, but I think that
IntoRaw{Fd,Handle,Socket}
may want to be its own RFC.