Fix the exit condition for Subprocess

[andrew-stripe]

Subprocess a bug in which it can fail to read data buffered in pipes after the process terminates. It currently attempts to handle this case by calling drain_fd on each ready fd after detecting that the process has exited by calling poll. If the process writes to the pipe between the select and the poll, we won't attempt to read from that pipe. This can happen if the process write to stderr, then writes to stdout and then exits. select will return after the write to stderr then the child writes to stdout and exits and then we poll. I think there's also an edge case where the process receives an interupt in drain_fd after the child has exited and catches EINTR.

Instead we should continue looping until we've received EOF or EPIPE from each of the read pipes except the self_pipe. We can remove the termination condition that waits for wait_r.length == 0 since that will never be reached since the self_pipe is never removed from wait_r and should never be nil.

[andrew-stripe]

r? @russell-stripe @doy-stripe cc @stripe-internal/product-infra @carl-stripe

[russell-stripe]

Other than that,

👍

[andrew-stripe]

re-r? @russell-stripe @doy-stripe (note the change in behavior around writes after the process has exited)

[andrew-stripe]

Note that swallowing the EPIPE on writes is consistent with Open3 from the Ruby stdlib: https://github.com/ruby/ruby/blob/ruby_2_3/lib/open3.rb#L313-L316

[russell-stripe]

A test for the EPIPE scenario would be nice. No need to block on it or re-r.

👍

[doy-stripe]

(swallowing EPIPE is also consistent with python's subprocess: https://github.com/python/cpython/blob/master/Lib/subprocess.py#L777 )

[andrew-stripe]

I tried writing a simple test for this but realized that it isn't trivial since it depends on a race between stdin being writable and the process exiting. I can't think of a way to test this reliably but I'm not going to merge this late on a Friday anyway so happy to consider other ideas on Monday!

[russell-stripe]

You should be able to test it by closing stdin in the subprocess and then sleeping or waiting on some other pipe (to prevent exiting).

[andrew-stripe]

Ah good call, this test works @russell-stripe.

[andrew-stripe]

Travis CI still hasn't started any of my builds from today. I manually ran the test suite twice on OS X and Ubuntu and confirmed it still passes.