Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

[Jeff King <peff@peff.net>]

On Sun, Aug 07, 2022 at 12:15:06PM +0200, René Scharfe wrote:

> > This adds "error: pumping io failed: No space left on device" to output.
> > Which kinda makes sense: With the pipe no longer blocking, there can be
> > a moment when the buffer is full and writes have to be rejected.  This
> > condition should be reported with EAGAIN, though.
> >
> > Adding "if (len < 0 && errno == ENOSPC) continue;" after the xwrite()
> > call in pump_io_round() lets the test pass.
> >
> > Perhaps the translation from Windows error code to POSIX is wrong here?
> 
> So if we fix that with the patch below, t3701.57 still hangs, but this
> time it goes through wrapper.c::handle_nonblock() again and again.
> Replacing the "errno = EAGAIN" with a "return 0" to fake report a
> successful write of nothing instead lets the test pass.
> 
> This seems to make sense -- looping in xwrite() won't help, as we need
> to read from the other fd first, to allow the process on the other end
> of the pipe to make some progress first, as otherwise the pipe buffer
> will stay full in this scenario.  Shouldn't that be a problem on other
> systems as well?

It doesn't happen on Linux; I suspect there's something funny either
about partial writes, or about poll() on Windows. What's supposed to
happen is:

  1. pump_io() calls poll(), which tells us the descriptor is ready to
     write

  2. we call xwrite(), and our actual write() call returns a partial
     write (i.e., reports "ret" bytes < "len" we passed in)

  3. we return back to pump_io() do another round of poll(). If the
     other side consumed some bytes from the pipe, then we may get
     triggered to do another (possibly partial) write. If it didn't, and
     we'd get EAGAIN writing, then poll shouldn't trigger at all!

So it's weird that you'd see EAGAIN in this instance. Either the
underlying write() is refusing to do a partial write (and just returning
an error with EAGAIN in the first place), or the poll emulation is wrong
(telling us the descriptor is ready for writing when it isn't).

Can you instrument pump_io_round() (or use some strace equivalent, if
there is one) to see if we do a successful partial write first (which
implies poll() is wrong in telling us we can write more for the second
round), or if the very first write() is failing (which implies write()
is wrong for returning EAGAIN when it could do a partial write).

-Peff