Closed ummarikar closed 4 years ago
Thanks for digging in!
macOS still only implements poll(2)
sort of ok for a relatively small number of file like objects in the kernel--I ran into issues with ptys as recently as within the past year, so I don't think we can safely take this change.
Thanks also for sharing the code that led you to look into this. I think I can see the issue in that code; this line is the problem:
with more context shown here:
loop {
...
if pollfds[1].revents & POLLIN == POLLIN {
while let Ok(i) = stdout.read(&mut buf) {
if i == 0 {
break;
}
...
}
}
When a read of a pipe returns 0 it indicates EOF and is equivalent to the POLLHUP signal that is being tested elsewhere in that code. The code breaks out of the inner loop in the 0-byte-read case, but not the outer loop, so it continues to get a 0 byte result on each iteration.
It's not necessary to have the inner loop; in fact, that inner loop can cause you the opposite problem: since the pipes are not set to non-blocking mode, a second iteration and attempt to read can block and defeat the polling.
To fix this I'd recommend re-structuring the function along these lines, which avoids making several duplicate file descriptors, avoids busy looping when either one of the pipes has reached EOF, and has better handling of utf8 text when inner.nocapture
is true:
fn run_cmd(..., mut cmd: Command) -> (ExitStatus, String, String) {
let mut child = cmd
.stderr(process::Stdio::piped())
.stdout(process::Stdio::piped())
.stdin(process::Stdio::null())
.spawn()
.unwrap_or_else(|_| fatal(&format!("Couldn't run command {:?}.", cmd)));
// Convert the pipes into FileDescriptor objects.
let mut stdout: FileDescriptor = child.stdout.into();
let mut stderr: FileDescriptor = child.stderr.into();
// Buffers to accumulate the child output
let mut output = Vec::new();
let mut error = Vec::new();
let mut pollfds = vec![
pollfd {
fd: stdout.as_socket_descriptor(),
events: POLLIN | POLLHUP,
revents: 0,
},
pollfd {
fd: stderr.as_socket_descriptor(),
events: POLLIN | POLLHUP,
revents: 0,
},
];
while !pollfds.is_empty() {
if !poll(pollfds.as_mut_slice(), Some(timeout)).is_some() {
continue;
}
// We want to mutate the `pollds` vector, but we can't
// do that while iterating over it, so we make a copy.
// Since we may want to remove an entry, we process the
// entries in reverse order so that the indices remain stable
for (idx, item) in pollfds.cloned().iter().rev().enumerate() {
if item.revents == 0 {
// Nothing ready
continue;
}
// Figure out if this is stdout or stderr
let (pipe, target_buf, is_stdout) = if item.fd == stdout.as_socket_descriptor() {
(&mut stdout, &mut output, true)
} else {
(&mut stderr, &mut error, false)
};
let mut buf = [0u8; READBUF];
match pipe.read(&mut buf) {
// EOF
Some(0) => pollfds.remove(idx),
// Stop reading a stream if we encounter an error on it
Err(err) => {
eprintln!("Error reading from child:{}", err);
pollfds.remove(idx);
}
Some(n) => {
let buf = &buf[..n];
target_buf.extend_from_slice(buf);
if inner.nocapture {
// They want to print the data; pass through
// to our out/error stream as appropriate
if is_stdout {
std::io::stdout().write_all(buf).unwrap();
} else {
std::io::stderr().write_all(buf).unwrap();
}
}
}
}
}
}
let stdout = String::from_utf8(output).unwrap();
let stderr = String::from_utf8(error).unwrap();
let status = {
// wait on the child and get its status here
};
(status, stdout, stderr)
}
I believe poll(2) on macOS is no longer broken on the latest versions of macOS and that can be seen by running the C code in this article - https://daniel.haxx.se/blog/2016/10/11/poll-on-mac-10-12-is-broken/.
FYI I am a student at King's College London working on an interpreter in Rust and we use a library called lang_tester. While testing the interpreter on macOS I run into an infinite loop which is not the case on Linux. This is the culprit: https://github.com/softdevteam/lang_tester/blob/a1f51ec8b747cb252a0b8df4846ff43c8c0c6c8b/src/tester.rs#L729-L731 After looking in filedescriptor I saw that because select(2) is being used, there is no implementation of POLLHUP so the if statement above never enters. When removing the macOS specific code, lang_tester runs fine and doesn't enter the loop mentioned before. Hence why I have submitted this PR. If by using select(2) you are avoiding another issue with poll(2) which I haven't considered, please let me know as I am new to this and in that case feel free to close this PR!