Re: [PATCH] mingw: handle writes to non-blocking pipe

[Jeff King <peff@peff.net>]

On Thu, Aug 11, 2022 at 07:35:33PM +0200, René Scharfe wrote:

> OK, but we can't just split anything up as we like: POSIX wants us to
> keep writes up to PIPE_BUF atomic.  When I read that name I mistakenly
> thought it was the size of the pipe buffer, but it's a different value.
> The minimum value according to POSIX is 512 bytes; on Linux it's 4KB.
> 
> And Windows doesn't seem to define it.  Bash's ulimit -p returns 8,
> which is in units of 512 bytes, so it's 4KB like on Linux.  But that's
> apparently not respected by Windows' write.
> 
> I just realized that we can interprete the situation slightly
> differently.  Windows has effectively PIPE_BUF = 2^32, which means all
> writes are atomic.  I don't see POSIX specifying a maximum allowed
> value, so this must be allowed.  Which means you cannot rely on write
> performing partial writes.  Makes sense?

Hmm, I'm not sure. POSIX does allow writing a single byte if that's all
the space there is, but only if the original _request_ was for more than
PIPE_BUF. Which I guess matches what you're saying.

If the original request is smaller than PIPE_BUF, it does seem to say
that EAGAIN is the correct output. But it also says:

  There is no exception regarding partial writes when O_NONBLOCK is set.
  With the exception of writing to an empty pipe, this volume of
  POSIX.1-2017 does not specify exactly when a partial write is
  performed since that would require specifying internal details of the
  implementation. Every application should be prepared to handle partial
  writes when O_NONBLOCK is set and the requested amount is greater than
  {PIPE_BUF}, just as every application should be prepared to handle
  partial writes on other kinds of file descriptors.

  The intent of forcing writing at least one byte if any can be written
  is to assure that each write makes progress if there is any room in
  the pipe. If the pipe is empty, {PIPE_BUF} bytes must be written; if
  not, at least some progress must have been made.

So they are clearly aware of the "we need to make forward progress"
problem. But how are you supposed to do that if you only have less than
PIPE_BUF bytes left to write? And likewise, even if it is technically
legal, having a PIPE_BUF of 2^32 seems like a quality-of-implementation
issue.

And that doesn't really match what poll() is saying. The response from
poll() told us we could write without blocking, which implies at least
PIPE_BUF bytes available. But clearly it isn't available, since the
write fails (with EAGAIN, but that is equivalent to blocking in POSIX's
view here).

Lawyering aside, I think it would be OK to move forward with cutting up
writes at least to a size of 512 bytes. Git runs on lots of platforms,
and none of the code can make an assumption that PIPE_BUF is larger than
that. I.e., we are reducing atomicity provided by Windows, but that's
OK.

I don't think that solves our problem fully, though. We might need to
write 5 bytes, and telling mingw_write() to do so means it's supposed to
abide by PIPE_BUF conventions. But again, we are in control of the
calling code here. I don't think there's any reason that we care about
PIPE_BUF atomicity. Certainly we don't get those atomicity guarantees on
the socket side, which is where much of our writing happens, and our
code is prepared to handle partial writes of any size. So we could just
ignore that guarantee here.

> If we accept that, then we need a special write function for
> non-blocking pipes that chops the data into small enough chunks.

I'm not sure what "small enough" we can rely on, though. Really it is
the interplay between poll() and write() that we care about here. We
would like to know at what point poll() will tell us it's OK to write().
But we don't know what the OS thinks of that.

Or maybe we do, since I think poll() is our own compat layer. But it
just seems to be based on select(). We do seem to use PeekNamedPipe()
there to look on the reading side, but I don't know if there's an
equivalent for writing.

> Another oddity: t3701 with yesterday's patch finishes fine with -i, but
> hangs without it (not just when running it via prove).  O_o

Yes, definitely strange. I'd expect weirdness with "-v", for example,
because of terminal stuff, but "-i" should have no impact on the running
environment.

-Peff