Reduce stdin/stdout deadlocks

[epage]

From @luser on #28

You actually might want to consider refactoring the current code to write stdin on a separate thread and then read stdout/stderr in parallel to avoid possible deadlock. Right now if the spawned process produces enough output to fill up its stdout or stderr buffers it'll block trying to write those, and if you are still writing stdin data you'll be blocked as well. I've got a wait_with_input_output function that does this in sccache which you can copy if you'd like. (Linking to an older version because the current source uses tokio-io and futures.)

So far, we haven't run into this but

[luser]

It's not hard to create a testcase that hits this, it's just probably unlikely to happen in common scenarios because pipe buffers are reasonably large (65k by default on Linux, >100k with large writes on macOS). Here's a testcase that deadlocks on my mac (just cargo run it): https://github.com/luser/stdin-stdout-deadlock-test .

[shssoichiro]

I just ran into this writing a test for a project I'm on. Once I found this issue, the workaround was simple (use a smaller test data), but unexpected to follow the example and get an obscure "Broken Pipe" error.

[epage]

@luser , I was looking at adopting your solution from sccache but something looks like a bug to me

    stdin.and_then(|t| t.join().unwrap().ok());
    let stdout = child.take_stdout().map(read);
    let stderr = child.take_stderr().map(read);

sccache block on the stdin thread before spawning the stdout/stderr threads. Shouldn't those lines be swapped?

    let stdout = child.take_stdout().map(read);
    let stderr = child.take_stderr().map(read);
    stdin.and_then(|t| t.join().unwrap().ok());

[luser]

sccache block on the stdin thread before spawning the stdout/stderr threads. Shouldn't those lines be swapped?

I suspect you're right, good catch. The ordering of joining the stdin thread before calling wait is important as noted in the code but running the stdin writer + stdout/stderr writer threads in parallel would be the best way to ensure that the child process doesn't get stuck because a pipe buffer is full.