add std::os::unix::process::CommandExt::fd by Qelxiros · Pull Request #145687 · rust-lang/rust (original) (raw)
r? @tgross35
rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
This comment has been minimized.
Comment on lines 52 to 111
| impl CommandExt for Command { |
|---|
| fn fd(&mut self, new_fd: RawFd, old_fd: impl Into<OwnedFd>) -> &mut Self { |
| let old = old_fd.into().as_raw_fd(); |
| unsafe { |
| self.as_inner_mut().pre_exec(Box::new(move | |
| cvt_r(| |
| let flags = cvt(libc::fcntl(new_fd, F_GETFD))?; |
| cvt(libc::fcntl(new_fd, F_SETFD, flags & !FD_CLOEXEC))?; |
| cvt_r(| |
| Ok(()) |
| })) |
| } |
| self |
| } |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using pre_exec for this can reduce the performance of starting the child process.
See my Zulip thread on this:
#t-libs-api/api-changes > Extra FDs in CommandExt @ 💬
also mentioned in this comment in the tracking issue:
#144989 (comment)
Using pre_exec switches the spawn implementation from posix_spawn to fork + execve syscalls. Forking is slower than posix_spawn, because it needs to copy the programs memory (in practice copy on write optimizations are used, but they are still somewhat costly - see my benchmarks also linked in the Zulip message). posix_spawn supports setting the passed FDs, so this feature should be used if possible.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you happen to have a sketch of what a better interface would look like?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but the issue I've described here is not a problem with the signature of fd. The extension should not use the public pre_exec method, the implementation needs to be modified instead.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this file is of interest:
| if let Some(ret) = self.posix_spawn(&theirs, envp.as_ref())? { |
|---|
| return Ok((ret, ours)); |
| } |
Note that there are multiple structs called Command because of how the target-specific functionality is implemented.
Command.spawn first tries to run the posix_spawn method, but its implementations return None if an attribute which the limited posix_spawn syscall cannot handle is set - see the implementations of the posix_spawn method.
Please keep this behavior in mind when adding new Command attributes and creating tests - cases when spawn uses the posix_spawn syscall and when it does not should both be tested. I'm not sure if there is a way to check which spawn variant was chosen in the tests, it is an implementation detail after all.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extension should not use the public
pre_execmethod, the implementation needs to be modified instead.
See how other methods in the CommandExt trait are implemented in the library/std/src/os/fd/process.rs file that you modified - they all call the actual implementation using .as.inner_mut()
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be reasonably straightforward to add a Vec<(OwnedFd, RawFd)> to Command that could be used either with something based on posix_spawn_file_actions_adddup2 or with the exec fallback (also avoiding 1 closure per fd). But aside from being slow, is there any correctness problem with the current implementation?
As long as there isn't anything explicitly wrong, I think it would be reasonable to get the current implementation over the line first so we have some tests. Any chance you would be interested in submitting a followup changing the implementation, since you have been looking into this a lot?
I'm not sure if there is a way to check which spawn variant was chosen in the tests, it is an implementation detail after all.
Adding a last_spawn_was_posix_spawn field to Command for debugging would be fine. That will be accessible from tests once they're moved to within std/src.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But aside from being slow, is there any correctness problem with the current implementation?
No, I'm not claiming this implementation is incorrect - I also haven't reviewed it thoroughly though.
Any chance you would be interested in submitting a followup changing the implementation, since you have been looking into this a lot?
If I have the time, sure.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
fd_test_swap is broken; I'm not confident that swapping fds like that is possible without a dedicated function since they seem to clobber each other. fd_test_close_time is also broken, at least on my machine. I'm not sure why that is, but I'd appreciate any guidance you may have.
This comment has been minimized.
This comment has been minimized.
fd_test_swapis broken; I'm not confident that swapping fds like that is possible without a dedicated function since they seem to clobber each other.
That's expected with the current implementation when both fds need to be alive at the same time; this is what we should check for. The goal is to make sure we don't quietly change this behavior by accident.
fd_test_close_timeis also broken, at least on my machine. I'm not sure why that is, but I'd appreciate any guidance you may have.
This one is unfortunately trickier. Linux aggressively reuses fds so it's getting mapped to something else after close, so I guess checking flags isn't enough. Instead, checking that the dev+ino are unequal should work:
use std::{fs, io}; use std::os::fd::AsRawFd; use std::os::unix::fs::MetadataExt;
#[test] fn whatever() { let (_pipe_reader, pipe_writer) = io::pipe().unwrap();
let fd = pipe_writer.as_raw_fd();
let fd_path = format!("/dev/fd/{fd}");
// let mut cmd = Command::new("cat") ...
// Get the identifier of the fd (metadata follows symlinks)
let fd_id = md_file_id(&fs::metadata(&fd_path).expect("fd should be open"));
// stand in for cmd.spawn().unwrap();
drop(pipe_writer);
// After the child is spawned, our fd should be closed
match fs::metadata(&fd_path) {
// Ok; fd exists but points to a different file
Ok(md) => assert_ne!(md_file_id(&md), fd_id),
// Ok; fd does not exist
Err(_) => ()
}
// ...}
/// Use dev + ino to uniquely identify a file fn md_file_id(md: &fs::Metadata) -> (u64, u64) { (md.dev(), md.ino()) }
Even when checking device/inode values, fd_test_close_time fails on my machine. I think that guarantees that the file descriptor is staying open longer than it should, but I have no idea why. I'm a little out of my depth here, but I'll try to look into it. Let me know if you think of anything else.
Ibraheem has a lot of reviews
r? libs
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors bot added a commit that referenced this pull request
add std::os::unix::process::CommandExt::fd
try-job: aarch64-apple try-job: arm-android try-job: test-various
This comment has been minimized.
It looks like the behavior of test_swap is platform-dependent. I don't have easy access to non-Linux machines for testing purposes, so I've removed the test.
@rustbot ready
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
| /// |
|---|
| /// [`posix_spawn`]: https://www.man7.org/linux/man-pages/man3/posix\_spawn.3.html |
| #[unstable(feature = "command_pass_fds", issue = "144989")] |
| fn last_spawn_was_posix_spawn(&self) -> Option<bool>; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem mentioned in the ACP or tracking issue, and seems like an odd place to put this. It's also not clear to me why this is on Command vs on Child?
I'm not convinced we want to make this a stable API -- posix_spawn feels like a low level detail rather than something callers should care about. If we do want this exposed we should explain what callers are supposed to do with the information.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for testing purposes (see #145687 (comment)). Would it be better to mark it perma-unstable? (I'm not sure exactly how to do that)
I don't think it can be private because tests are treated as separate crates.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to make it private. Integration tests in library/std/tests are treated as separate crates, but these are in src/ so can use internal API.
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.
Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
It looks like the behavior of test_swap is platform-dependent. I don't have easy access to non-Linux machines for testing purposes, so I've removed the test.
One failing platform certainly isn't a reason to remove a completely valid test entirely. Mark it #[cfg_attr(not(target_os = "android"), should_panic)] (and any other platforms that fail) with a FIXME, it will be worth seeing if we can fix this somehow given users are probably going to expect consistent behavior here.
| pipe_writer.write_all(b"Hello, world!").unwrap(); |
|---|
| drop(pipe_writer); |
| child.wait().unwrap(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| child.wait().unwrap(); |
|---|
| child.wait().unwrap().exit_ok().unwrap(); |
Verify the child didn't run into errors, applies a few places