RFC for redirecting stdio of child processes to open file handles

[ipetkov]

Rendered

[ipetkov]

cc @alexcrichton

[alexcrichton]

Could you add some details on how this structure is going to be created as well? (the more detailed an RFC is the better!)

Also, I think that File may not be the best type to use here, or at least it may depend on the implementation. I would guess that on unix this would use the pipe system call, but File has methods like set_len which may not work out too well for pipes.

As a final point, could you add some words as to how this will be implemented on Windows as well?

[ipetkov]

@alexcrichton I think you are right that File isn't the correct type to use here. I was thinking as a File as an abstraction over a file descriptor/HANDLE, but I didn't really think about the distinctions between pipes and files. I'll update the design with a separate wrapper for pipes to maintain those distinctions!

[rschmitt]

Isn't this what InheritFd did in old_io? Was that also implemented on Windows?

[alexcrichton]

When directing to another process, the pipe actually already exist in a sense where Stdio::piped() means that the child will be created with a stdin/stdout/stderr handle, under which is just a file descriptor. That may end up being more useful in the long run to hook up processes together perhaps?

[ipetkov]

It's true that the pipes exist internally, but they only pipe between the parent and child only. Although an API for hooking up several processes directly may be useful, I think that extending the current API with this functionality may be too troublesome to make it truly flexible, besides simply allowing the caller to pass in some sort of file descriptor and let them worry about hooking things up properly.

[alexcrichton]

Overall I like the direction that this is going in, but I'm hesitant with how it interacts with other aspects of the system. For example this interface should largely boil down to "just passing a file descriptor" on unix and something like "just passing a HANDLE" on windows. Right now the restriction of using a File itself may be too limiting, and I'm not quite so sure about borrowed-vs-owned aspect of the argument just yet, I think I'll need to ponder this a bit more.

Regardless though thanks for the RFC!

[ipetkov]

@rschmitt Yeah InheritFd allowed redirection of arbitrary file descriptors, even on Windows, except the caller would have to manually convert the HANDLE as an fd via libc calls themselves. Not to mention the unsafety of the file descriptor getting closed at some point before the command was actually spawned (or worse a different file opened under that same descriptor). This RFC seeks to make things a bit easier and safer when redirecting stdio!

@alexcrichton I agree that using a File is too limiting, so I've updated my design to create a new handle for pipes.

For example this interface should largely boil down to "just passing a file descriptor" on unix and something like "just passing a HANDLE" on windows.

I think this would be a good approach for the future, but I wanted to avoid Unix/Windows discrepancies in the API around sockets (they aren't quite HANDLES on Windows and I'm not sure if they can be substituted for each other) so I thought about using a "white list" of accepted types for redirection. Although not as flexible as accepting the raw OS handle type it seems like a decent compromise for now.

[lilyball]

Using an Rc removes the Send and Sync bounds on Stdio, which is a breaking change.

[ipetkov]

I suppose this would have to be Arc then?

[lilyball]

Yeah, that should work.

[alexcrichton]

I feel that this may be a bit ambitious for the standard library, it's pretty rare to see an Arc under the hood of a primitive for the purpose of implementing Clone right now. There's definitely a use case of being able to share descriptors among spawned processes, but I think that we may want to expose that via raw handles for now.

For example unix could have Stdio::from_raw_fd and windows could have Stdio::from_raw_handle which will be low-level wrappers to pass I/O handles to the child. This would satisfy the ability to share the same handle among many processes (probably unsafely) for now as it's probably a somewhat rare desire.

[ipetkov]

I think the unsafety should be acceptable for now, especially since commands are probably executed as soon as they are built and all handles are still in scope.

Also I think it might be better to simply have a redirect method defined to take AsRawFd implementers on Unix, and AsRawHandle implementers on Windows to make usage of the API more portable. I'll update the proposal with the changes.

[alexcrichton]

Also I think it might be better to simply have a redirect method defined to take AsRawFd implementers on Unix, and AsRawHandle implementers on Windows to make usage of the API more portable

You've mentioned this in the RFC as written, but this'll need to be a platform-specific extension as the set of types implementing AsRawFoo is different across platforms.

[ipetkov]

I was thinking something along the lines of a StdioExt trait which will be defined in std::os::$patform::io based on the appropriate AsRawFoo, but their method signatures would match lexically.

// Unix
pub trait StdioExt { fn redirect<T: AsRawFd>(t: &T) -> Stdio { ... } }
// Windows
pub trait StdioExt { fn redirect<T: AsRawHandle>(t: &T) -> Stdio { ... } }

So then you could write something like the following in both Unix and Windows

#[cfg(unix)] use std::os::unix::io::StdioExt as StdioExt;
#[cfg(windows)] use std::os::windows::io::StdioExt as StdioExt;

let file = File::open(...);
let stdio = unsafe { StdioExt::redirect(file) };
...

Instead of having to write the following everywhere you want to redirect something

#[cfg(unix)] use std::os::unix::io::StdioExt as StdioExt;
#[cfg(windows)] use std::os::windows::io::StdioExt as StdioExt;

let file = File::open(...);
let stdio = unsafe {
    if cfg!(unix) {
        StdioExt::from_raw_fd(file)
    } else {
        StdioExt::from_raw_handle(file)
    }
};
...

This way users are still somewhat aware that this is platform specific code, without suffering the constant need of cfg.

[alexcrichton]

Interesting! Definitely sounds like a plausible idea to me!

[ipetkov]

I've updated and simplified the design to work with ChildStd{in, out, err} which are already using OS pipes to talk with the child process.

I've left out a public interface for safely creating OS pipes out of this version because it doesn't seem as necessary any more. It would be pretty trivial to expose a public wrapper over AnonPipe either now or in the future should this become a requested feature.

[alexcrichton]

This was actually recently fixed in rust-lang/rust#24426 as well, so yay!

[alexcrichton]

This looks great to me @ipetkov, thanks!

I'm a tad bit uncertain about whether unsafe is the right way to go here, but I think that it's the best course of action. Right now we're providing the guarantee that a File is the sole and unique owner of that file descriptor/handle, hence why from_raw_fd is now unsafe. If we were to make Stdio::redirect safe, then you could create the redirection, drop the source handle, open a new handle, and then spawn the child. This newly opened handle would then be used by the child process, violating the unique ownership semantics of File. As a result I think it's fine to call the API unsafe.

cc @nagisa, @retep998, @carllerche, @aturon

[l0kod]

👍 We definitely need this features to replace the lost std::old_io::pipe::PipeStream and StdioContainer::InheritFd.

The Redirect(RawFd) could be replaced with something like Redirect<T: AsRawFd>(&mut T) to be able to not need unsafe code: exclusive use of the file descriptor/handle during all it's lifetime.

StdioExt::redirect<T: AsRawFd>(t: &'a mut T) should return a Stdio with the 'a lifetime.

We should be able to connect the stdio/pipes before spawning any command.

Agree with @alexcrichton to no use File but something lighter. I previously proposed to expose something like FileDesc but common between UNIX and Windows (e.g. IoHandle, cf rust-lang/rust#15643) to keep a common function signature whatever OS is used. However, &mut As{RawFd,Handle} could do the job.

[alexcrichton]

@l0kod the recommended unsafety here derives from the fact that there's no lifetime associated with the return value (see the section about backwards compatibility and my prior comment). It also already does take T: AsRawFd as Stdio::Redirect is not an expose enum variant (it's only exposed through an extension trait).

[ipetkov]

We should be able to connect the stdio/pipes before spawning any command.

@l0kod That's one argument for exposing a public Pipe interface which wraps over AnonPipe and would allow setting that all up before spawning, which would be pretty easy to add

[l0kod]

the recommended unsafety here derives from the fact that there's no lifetime associated with the return value

Well, I suggest to add a lifetime to the return value: rust-lang/rust#24251 (comment)

[ipetkov]

@l0kod I think @alexcrichton is referring to Stdio as the return value, and not the raw handle type. Stdio is stored by Command, so if it was to get a lifetime parameter due to AsRaw{Fd, Handle}, so would Command. At this point this would be a breaking change of a stabilized API.

[l0kod]

I didn't realize this was so impacting. I know Rust has a big part stabilized but I understood that some (rare) breaking changes could happen before the final 1.0.0
@alexcrichton Is it really too late to fix this objects' lifetimes?

[ipetkov]

This would take the (File) ownership and could get ride of the unsafe.

What about redirecting multiple child processes to the same File? Then either Stdio would have to implement Clone, or anything else that holds a file descriptor/HANDLE.

[l0kod]

What about redirecting multiple child processes to the same File? Then either Stdio would have to implement Clone, or anything else that holds a file descriptor/HANDLE.

A Stdio::tee(&self) -> Stdio could be in charge of writing multiple time to the same file description (i.e. Stdio). It seems appropriate to use tee(2) in the Stdio implementation for that.
This way, we can control and handle potentially unsafe operations in only one implementation.

[ipetkov]

I've updated the proposal to consider FromRaw* as an implementation and proposed several approaches for a higher level API. After thinking about the tradeoffs for each approach, I'm still a fan of using StdioExt (because I find it cleaner than using an intermediary trait), however, I am open to other ideas!

@l0kod Is tee(2) available across platforms (e.g. Windows)?

[rschmitt]

In the meantime, is there a workaround we can use on nightly and/or stable? Heatseeker has only been usable on Windows ever since old_io was removed (relevant code).

[retep998]

@ipetkov I cannot find tee(2) or equivalent on Windows.

[ipetkov]

@rschmitt there are two workarounds that come to mind:

• Spawn a thread to buffer data from the given descriptor to the child's stdin, but probably won't scale very well
• dup2 the given descriptor to the parent's stdin fd and have the child inherit that

[rschmitt]

@ipetkov Your second suggestion is exactly what I needed. Works great for what I need, sure beats doing the ioctls by hand. Thanks!

[l0kod]

Is tee(2) available across platforms (e.g. Windows)?

Even if tee(2) is not available, the Stdio could implement a tee() -> Stdio and use something similar to tee(2) internally (e.g. DuplicateHandle or manual copy?).

[alexcrichton]

@ipetkov could you update this RFC with specific signatures for the functions you have in mind? I've found it useful to have the signatures in mind ahead of time as it makes it easier to discuss portions of the API in terms of generics and ownership.

[ipetkov]

@l0kod To my understanding of the tee syscall on Linux it doesn't quite behave like simply duplicating a descriptor, so it may prove difficult to emulate that functionality on other systems (lest we introduce confusion by using the same name).

Internal duplication of the OS handle is definitely a possibility, however, I personally lean towards allowing the caller to choose if they want duplication (and expecting them to use dup themselves) rather than forcing it on them.

@alexcrichton Updated the RFC!

[alexcrichton]

I would recommend structuring this RFC to make a specific recommendation on which API route to choose and leave the other two to the "Alternatives" section. I would personally recommend something like:

1. Having an extra StdioExt struct seems somewhat odd as it's not actually returned or needed to be a struct, so I'd list this as an alternative.
2. Using an extension trait is currently the standard method for extending functionality on a type, so I'd recommend listing this as the what the RFC proposes.
3. I would recommend listing this as an alternative, but definitely emphasize the non-cross-platform aspect as it's the reason I wouldn't want to pursue it.

[ipetkov]

Updated the RFC to recognize the FromRaw* implementations on Stdio and to reflect @alexcrichton 's suggestions of the previously offered design alternatives

[alexcrichton]

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) doing Stdio::from_raw_fd(file.into_raw_fd()). I suspect that adding traits like IntoRawFd are somewhat inevitable.

[alexcrichton]

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.

[ipetkov]

@alexcrichton

the contract of FromRawFd is that it takes ownership of the file descriptor (or handle) in question

Ah, I had overlooked this...

However, doesn't this require that the caller mem::forget the wrapper over the descriptor/HANDLE or at least manually dup it (and perform error checking)? Because the contract of AsRawFd does not transfer ownership and then both Stdio and the original wrapper will both try to close the handle.

I suspect that adding traits like IntoRawFd are somewhat inevitable.

I also agree, however, I feel like something like IntoRawFd could be very useful now, because being forced to mem::forget or dup 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 the Command interface is so handy to have).

The alternative (IntoRawFd) I don't think is unergonomic enough to warrant a specialized function.

I could definitely live with a compromise like IntoRawFd, but a specialized function could avoid accidental use of AsRawFd instead of IntoRawFd and prevent someone from shooting their foot (since both traits would produce a fd which FromRawFd will happily accept).

Assuming the availability of an IntoRawFd trait the currently proposed API could be tweaked to use it more appropriately:

pub trait StdioExt {
   // Take ownership of the handle
   fn redirect<T: IntoRaw*>(t: T) -> Stdio;
   // Unsafely borrow the handle, letting caller ensure it is valid
   unsafe fn redirect_by_ref<T: AsRaw*>(t: &T) -> Stdio;
}

[alexcrichton]

However, doesn't this require that the caller mem::forget the wrapper over the descriptor/HANDLE or at least manually dup it (and perform error checking)?

I believe this is correct, yes.

Assuming the availability of an IntoRawFd trait the currently proposed API could be tweaked to use it more appropriately:

I think one part of this that I'm uneasy about is how Stdio::redirect(t) is the same as Stdio::from_raw_fd(t.into_raw_fd()). Along those lines it seems redundant to have the redirect method, and then only adding an unsafe redirect_by_ref method also seems somewhat out of place because it's relatively the same thing as Stdio::from_raw_fd(f.as_raw_fd()) + a mem::forget somewhere.

[ipetkov]

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.

[alexcrichton]

Ok, in that case it may be good motivation to push harder on these traits perhaps?

[ipetkov]

I believe so. Do you think this RFC would be an appropriate place to discuss them?

[alexcrichton]

I think this would serve as good motivation, but I think that IntoRaw{Fd,Handle,Socket} may want to be its own RFC.

[ipetkov]

Based on @alexcrichton's feedback I've written up RFC #1174 to discuss adding new traits such as IntoRaw{Fd, Socket, Handle}.

This RFC still retains its cross-platform benefits over using IntoRaw*, however, if there is no strong desire for a cross-platform redirection API at the time, we can close this RFC in favor of #1174.

[alexcrichton]

This RFC is now entering its week long final comment period.

[SimonSapin]

#[cfg(unix)] use std::os::unix::process::StdioExt;
#[cfg(windows)] use std::os::windows::process::StdioExt;

If some form of StdioExt::redirect is available on all supported platforms, the above is just noise IMO. I’d much prefer redirect being in inherent method of Stdio, as proposed in Alternatives.

I’d go even further: rather than have redirect itself have a platform-specific signature, it could use a new trait (AsStdioRedirect?) that internally uses eiher AsRawFd or AsRawHandle.

[sfackler]

@SimonSapin a given type may not implement both AsRawFd on Unix and AsRawHandle on Windows (e.g. TcpStream which implements AsRawSocket instead of AsRawHandle). Having to import the StdioExt traits makes people opt into potential platform specific breakage.

Taking an AsRaw* by value and then storing it away seems a bit weird - particularly since it's not unreasonable to have non-owning types that implement AsRaw*. I'd be in favor of accepting #1174 and using that instead.

[alexcrichton]

The consensus of the libs team on this RFC is to close this in favor of the IntoRawFd RFC, so I'm going to close this in favor of that. Thanks regardless though for the RFC @ipetkov!

[ipetkov]

Thanks for all the feedback on this RFC anyway @alexcrichton!