RFC patchpipe_command(): mark stdin descriptor as non-blocking

Our pipe_command() helper lets you both write to and read from a child process on its stdin/stdout. It's supposed to work without deadlocks because we use poll() to check when descriptors are ready for reading or writing. But there's a bug: if both the data to be written and the data to be read back exceed the pipe buffer, we'll deadlock.

The issue is that the code assumes that if you have, say, a 2MB buffer to write and poll() tells you that the pipe descriptor is ready for writing, that calling:

  write(cmd->in, buf, 2*1024*1024);

will do a partial write, filling the pipe buffer and then returning what it did write. And that is what it would do on a socket, but not for a pipe. When writing to a pipe, at least on Linux, it will block waiting for the child process to read() more. And now we have a potential deadlock, because the child may be writing back to us, waiting for us to read() ourselves.

An easy way to trigger this is:

  git -c add.interactive.useBuiltin=true \
      -c interactive.diffFilter=cat \
      checkout -p HEAD~200

The diff against HEAD~200 will be big, and the filter wants to write all of it back to us (obviously this is a dummy filter, but in the real world something like diff-highlight would similarly stream back a big output).

If you set add.interactive.useBuiltin to false, the problem goes away, because now we're not using pipe_command() anymore (instead, that part happens in perl). But this isn't a bug in the interactive code at all. It's the underlying pipe_command() code which is broken, and has been all along.

We presumably didn't notice because most calls only do input _or_ output, not both. And the few that do both, like gpg calls, may have large inputs or outputs, but never both at the same time (e.g., consider signing, which has a large payload but a small signature comes back).

The obvious fix is to put the descriptor into non-blocking mode, and indeed, that makes the problem go away. Callers shouldn't need to care, because they never see the descriptor (they hand us a buffer to feed into it).

Signed-off-by: Jeff King <peff@peff.net>
---
+cc Dscho for two reasons:

  - this is a fallout of the builtin add-interactive, though I again
    emphasize that it's just triggering a lurking bug (which is mine, no
    less!). But I thought you'd want to know.

  - more importantly, I'm not sure of the portability implications of
    the fix. This is our first use of O_NONBLOCK outside of the
    compat/simple-ipc unix-socket code. Do we need to abstract this
    behind a compat/ layer for Windows?

 run-command.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Jeff King <peff@peff.net> writes:

Thanks for a very well reasoned and explained patch.

Yup.  A very good question to ask for the platform maintainer.

On Tue, Aug 02, 2022 at 12:13:07AM -0400, Jeff King wrote:

So I think the answer is pretty clearly "yes", from the Windows CI results:

  run-command.c:1429:18: 'O_NONBLOCK' undeclared (first use in this function)
         flags |= O_NONBLOCK;
                  ^~~~~~~~~~

It looks like we'd have the option of either adding F_GETFL/F_SETFL support to compat/mingw.c's fake fcntl() function, or adding a compat/nonblock.c that abstracts the whole thing. I'd prefer the latter, as it makes the interface a bit narrower.

But I'm not sure what should go on the Windows side of that #ifdef. Unlike some other spots, I don't think we can just make it a noop, or Windows will be subject to the same deadlock (unless for some reason its write() does behave differently?).

-Peff

Jeff King <peff@peff.net> writes:

I agree that "make this non-blocking" is the preferred interface over "emuilate fcntl".

Let them deadlock so that they can fix it, and until then leave it a noop? That may break the CI tests for them so we could hide the known-to-be-broken test behind prerequisite to buy them time, perhaps?

On Tue, Aug 02, 2022 at 09:16:17AM -0700, Junio C Hamano wrote:

I was skeptical of including a test, but I was able to come up with a reproduction that triggers for me on Linux, but isn't too expensive to keep around.

Much to my surprise (and delight), it passes on the Windows CI with the noop! So I guess their pipes behave in the way we want by default.

It does feel like a bit of a landmine to have an enable_nonblock() function which might do nothing. I'm not sure if other descriptor types might behave differently on Windows. But hopefully the comment above the function is sufficient to make people think twice.

-- >8 --
Subject: [PATCH] pipe_command(): mark stdin descriptor as non-blocking

Our pipe_command() helper lets you both write to and read from a child process on its stdin/stdout. It's supposed to work without deadlocks because we use poll() to check when descriptors are ready for reading or writing. But there's a bug: if both the data to be written and the data to be read back exceed the pipe buffer, we'll deadlock.

The issue is that the code assumes that if you have, say, a 2MB buffer to write and poll() tells you that the pipe descriptor is ready for writing, that calling:

  write(cmd->in, buf, 2*1024*1024);

will do a partial write, filling the pipe buffer and then returning what it did write. And that is what it would do on a socket, but not for a pipe. When writing to a pipe, at least on Linux, it will block waiting for the child process to read() more. And now we have a potential deadlock, because the child may be writing back to us, waiting for us to read() ourselves.

An easy way to trigger this is:

  git -c add.interactive.useBuiltin=true \
      -c interactive.diffFilter=cat \
      checkout -p HEAD~200

The diff against HEAD~200 will be big, and the filter wants to write all of it back to us (obviously this is a dummy filter, but in the real world something like diff-highlight would similarly stream back a big output).

If you set add.interactive.useBuiltin to false, the problem goes away, because now we're not using pipe_command() anymore (instead, that part happens in perl). But this isn't a bug in the interactive code at all. It's the underlying pipe_command() code which is broken, and has been all along.

We presumably didn't notice because most calls only do input _or_ output, not both. And the few that do both, like gpg calls, may have large inputs or outputs, but never both at the same time (e.g., consider signing, which has a large payload but a small signature comes back).

The obvious fix is to put the descriptor into non-blocking mode, and indeed, that makes the problem go away. Callers shouldn't need to care, because they never see the descriptor (they hand us a buffer to feed into it).

Unfortunately O_NONBLOCK isn't available everywhere, especially on Windows. However, the included test seems to work fine there, which implies that pipes there do not behave in the same way (they will do the partial write by default, which is what we want). This is true even if I size up the diff for a larger pipe buffer (the value chosen here triggers the deadlock on Linux, but isn't too expensive to keep as a regression test).

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile                   |  1 +
 compat/nonblock.c          | 22 ++++++++++++++++++++++
 compat/nonblock.h          | 10 ++++++++++
 run-command.c              | 10 ++++++++++
 t/t3701-add-interactive.sh | 14 ++++++++++++++
 5 files changed, 57 insertions(+)
 create mode 100644 compat/nonblock.c
 create mode 100644 compat/nonblock.h

Am 03.08.22 um 05:53 schrieb Jeff King:

Isn't this test going to end here, reporting failure before it even gets to the interesting part?

On Wed, Aug 03, 2022 at 06:45:23PM +0200, René Scharfe wrote:

Urgh, whoops. That was from some last-minute tweaking of the comment. There was also a line:

  git diff --color | wc -c

before it so I could measure how big the output was for a few values.

It snuck into the emailed patch, but the actual test runs (including the Windows CI) didn't include that (since obviously they'd have failed the test).

Thanks for catching.

-Peff

Am 03.08.2022 um 19:20 schrieb Jeff King:

Without that line the added test hangs for me on the Git for Windows SDK on Windows 11.

With the patch below it fails and reports basically nothing:

   expecting success of 3701.57 'handle very large filtered diff':
           git reset --hard &&
           # The specific number here is not important, but it must
           # be large enough that the output of "git diff --color"
           # fills up the pipe buffer. 10,000 results in ~200k of
           # colored output.
           test_seq 10000 >test &&
           test_config interactive.diffFilter cat &&
           printf y >y &&
           force_color git add -p >output 2>&1 <y &&
           git diff-files --exit-code -- test

   HEAD is now at 095e8c6 main
   not ok 57 - handle very large filtered diff
   #
   #               git reset --hard &&
   #               # The specific number here is not important, but it must
   #               # be large enough that the output of "git diff --color"
   #               # fills up the pipe buffer. 10,000 results in ~200k of
   #               # colored output.
   #               test_seq 10000 >test &&
   #               test_config interactive.diffFilter cat &&
   #               printf y >y &&
   #               force_color git add -p >output 2>&1 <y &&
   #               git diff-files --exit-code -- test
   #
   1..57

The file "output" contains "error: failed to run 'cat'". This is add-patch.c::parse_diff() reporting that pipe_command() failed. So that's not it, yet. (I don't actually know what I'm doing here.)

---
 compat/nonblock.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:

Hmph. Interesting that it passes in CI, but not on your local setup. I wonder why pipes would behave differently. Or perhaps there is even some configuration different that means we are still running the perl add--interactive there, though I kind of doubt it (and couldn't find anything pointing there).

Still, if it fails in at least one spot, it's something we need to deal with. On the plus side, once we figure out how to fix it, the hand-grenade of "enable_nonblock() does nothing on Windows" will not be present anymore. ;)

That implies that your call to enable_nonblock() succeeded, or pipe_command() itself would have complained, too. Maybe instrument it like this:

Am 05.08.2022 um 17:36 schrieb Jeff King:

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?

Right, and the "Named" part should be explained: As [1] says: "[...] you can often pass a handle to an anonymous pipe to a function that requires a handle to a named pipe.".

And perhaps that's not the right thing to do after all, as [2] says: "Note that nonblocking mode is supported for compatibility with Microsoft LAN Manager version 2.0 and should not be used to achieve asynchronous input and output (I/O) [...]".

[1] https://docs.microsoft.com/en-us/windows/win32/ipc/anonymous-pipe-operations [2] https://docs.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-setnamedpipehandlestate

Am 05.08.2022 um 23:13 schrieb René Scharfe:

Side note: The above patch gives better insight on error, but also breaks t4205.105 (%(describe) vs git describe), t7400.16 (submodule add to .gitignored path fails) and t7406.59 (submodule update --quiet passes quietness to fetch with a shallow clone).

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?

René

---
 compat/mingw.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

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

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

write() on Windows reports ENOSPC when writing to a non-blocking pipe whose buffer is full and rejects writes bigger than the buffer outright. Change the error code to EAGAIN and try a buffer-sized partial write to comply with POSIX and the expections of our Git-internal callers.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/mingw.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Hi René,

On Wed, 10 Aug 2022, René Scharfe wrote:

Excellent analysis, thank you!

However, let's reword this to clarify that the error code is set to EAGAIN only if the buffer-sized partial write fails.

This constant hails all the way back from 897bb8cb2c2 (Windows: A pipe() replacement whose ends are not inherited to children., 2007-12-07), in case anyone wondered like I did where that number came from (and why it is so low).

It is outside the purview of this patch to change that constant, therefore I am fine with leaving this as-is.

It might make the code clearer to turn this into an `else if`.

It is probably easier to reason about to recurse instead of using a `goto` here.

Thank you for this patch! Dscho

On Wed, Aug 10, 2022 at 07:39:40AM +0200, René Scharfe wrote:

Bearing in mind that I have no qualifications for reviewing Windows-specific patches, this seems like the right thing to be doing from the behavior you saw.

One question:

OK, so we call GetNamedPipeInfo() to find the size of the pipe buffer. It's unclear to me from Microsoft's docs if that is the _total_ size, or if it's the remaining available size. Hopefully the latter, since none of this works otherwise. ;)

But two corner cases:

  - If we fail to get the size, we guess that it's the maximum. Is this
    dangerous? I'm not sure why the call would fail, but if for some
    reason it did fail and we can't make forward progress, would we
    enter an infinite recursion of mingw_write()? Would it be safer to
    bail with EAGAIN in such a case (through granted, that probably just
    puts us into an infinite loop in xwrite())?

  - According to the docs:

      If the buffer size is zero, the buffer is allocated as needed.

    If we see this case, we'd then call mingw_write() with 0 bytes,
    which I imagine also makes no forward progress (though maybe we
    eventually return a successful 0-byte write?).

-Peff

Am 10.08.2022 um 22:02 schrieb Jeff King:

AFAIU it's the total size, not the available space. I think I confused it with PIPE_BUF, which we should use instead.

Alternatively we could retry with ever smaller sizes, down to one byte, to avoid EAGAIN as much as possible. Sounds costly, though.

Ah, yes, forgot that case.

René

On Thu, Aug 11, 2022 at 12:34:46AM +0200, René Scharfe wrote:

Hmm. If it's giving us the total size, that seems like it may fail in a case like this:

  - we write a smaller amount to the pipe, say 7168 bytes. That leaves
    1024 bytes in the buffer. The reader reads nothing yet.

  - now we try to write another 4096 bytes. That's too big, so we get
    ENOSPC. But when we ask for the pipe size, it tells us 8192. So we
    _don't_ try to do a partial write of the remaining size, and return
    EAGAIN. But now we've made no forward progress (i.e., even though
    poll() said we could write, we don't, and we end up in the xwrite
    loop).

So we really do want to try to get a partial write. Even a single byte means we are making forward progress.

It's definitely not optimal, but it may not be too bad. If we cut the size in half each time, then at worst we make log2(N) extra write attempts, and we end up with a partial write within 50% of the optimal size.

-Peff

Am 11.08.2022 um 10:47 schrieb Jeff King:

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?

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

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

René

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

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.

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.

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

Am 11.08.2022 um 20:20 schrieb Jeff King:

Turns out that's not the case on Windows: 94f4d01932 (mingw: workaround for hangs when sending STDIN, 2020-02-17) changed the compatibility implementation to 'Make `poll()` always reply "writable" for write end of the pipe.'.

An updated version of the test helper confirms it (patch below) by reporting on Windows:

chunk size: 1 bytes
 EAGAIN after 8192 bytes
chunk size: 500 bytes
 EAGAIN after 8000 bytes
chunk size: 1000 bytes
 EAGAIN after 8000 bytes
chunk size: 1024 bytes
 EAGAIN after 8192 bytes
chunk size: 100000 bytes
 partial write of 8192 bytes after 0 bytes
 EAGAIN after 8192 bytes

On Debian I get this instead:

chunk size: 1 bytes
 POLLOUT gone after 61441 bytes
 EAGAIN after 65536 bytes
chunk size: 500 bytes
 POLLOUT gone after 60500 bytes
 EAGAIN after 64000 bytes
chunk size: 1000 bytes
 POLLOUT gone after 61000 bytes
 EAGAIN after 64000 bytes
chunk size: 1024 bytes
 POLLOUT gone after 62464 bytes
 EAGAIN after 65536 bytes
chunk size: 100000 bytes
 partial write of 65536 bytes after 0 bytes
 POLLOUT gone after 65536 bytes
 EAGAIN after 65536 bytes

So on Windows the POLLOUT bit is indeed never unset.

Based on the output above I think Linux' poll() won't consider a pipe writable that has less than PIPE_BUF (4096) available bytes.

This has been elusive so far (see 94f4d01932).

Perhaps we should take the advice about PIPE_NOWAIT in the docs serious and use overlapping (asynchronous) writes on Windows instead. This would mean reimplementing the whole pipe_command() with Windows API commands, I imagine.

It's especially grating because test runs also take very looong.

Avoiding xwrite() in pump_io_round() on top lets the test suite finish successfully.

René

---
 Makefile                 |  1 +
 t/helper/test-nonblock.c | 73 ++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c     |  1 +
 t/helper/test-tool.h     |  1 +
 4 files changed, 76 insertions(+)
 create mode 100644 t/helper/test-nonblock.c

On Sun, Aug 14, 2022 at 05:37:08PM +0200, René Scharfe wrote:

Ah, good find. That kind of explains everything, then, I think. ;)

Right, that makes sense. It would have to in order to meet the atomicity requirement for write(), but still always make forward progress for each write().

I wouldn't be opposed to that, in the sense that it's supposed to be a black box to the caller, and it's relatively small in size. But I think we're pretty close to having something usable without that, so I'd like to pursue a smaller fix in the interim.

That makes sense. We end up busy-looping between poll() and write() while we wait for our read descriptor to become available. But if poll() doesn't block, that's the best we can do.

-Peff

On Wed, Aug 17, 2022 at 01:39:32AM -0400, Jeff King wrote:

So here's what I came up with. Most of the changes are done as preparatory steps, so I believe the final commit should just work out of the box for you, as well as on Windows CI (which I just kicked off).

Here's a breakdown of the patches:

  [1/6]: compat: add function to enable nonblocking pipes
  [2/6]: nonblock: support Windows

This is my compat wrapper from v1 pulled into a separate function, and then your Windows implementation on top.

  [3/6]: git-compat-util: make MAX_IO_SIZE define globally available
  [4/6]: pipe_command(): avoid xwrite() for writing to pipe

This fixes the "loop forever" problem of using xwrite(), under the assumption that fixing our Windows poll() emulation is too hard. If we did fix it, then we could revert these (though keeping them can be thought of as a defensive measure against future bugs; busy-looping is less bad than deadlocking).

I _think_ the amount of busy-looping on Windows will be OK here. Any case that wouldn't deadlock currently is unaffected, and the cases that are will probably not be too wasteful. More discussion in the commit message.

  [5/6]: pipe_command(): handle ENOSPC when writing to a pipe

A hack around the ENOSPC problem you found. We could replace this with your mingw_write() patch, but it looked like that needed some corner cases to be smoothed a bit.

  [6/6]: pipe_command(): mark stdin descriptor as non-blocking

The actual fix, largely as before.

I don't love patches 3-5, but it seems like a practical path forward. I'd prefer to go with a minimal fix that removes the existing deadlock, and then we can do more cleanups on top. Thanks so much for all of your investigation and testing so far!

 Makefile                   |  1 +
 compat/nonblock.c          | 50 ++++++++++++++++++++++++++++++++++++++
 compat/nonblock.h          |  9 +++++++
 git-compat-util.h          | 22 +++++++++++++++++
 run-command.c              | 33 +++++++++++++++++++++----
 t/t3701-add-interactive.sh | 13 ++++++++++
 wrapper.c                  | 22 -----------------
 7 files changed, 123 insertions(+), 27 deletions(-)
 create mode 100644 compat/nonblock.c
 create mode 100644 compat/nonblock.h

-Peff

We'd like to be able to make some of our pipes nonblocking so that poll() can be used effectively, but O_NONBLOCK isn't portable. Let's introduce a compat wrapper so this can be abstracted for each platform.

The interface is as narrow as possible to let platforms do what's natural there (rather than having to implement fcntl() and a fake O_NONBLOCK for example, or having to handle other types of descriptors).

The next commit will add Windows support, at which point we should be covering all platforms in practice. But if we do find some other platform without O_NONBLOCK, we'll return ENOSYS. Arguably we could just trigger a build-time #error in this case, which would catch the problem earlier. But since we're not planning to use this compat wrapper in many code paths, a seldom-seen runtime error may be friendlier for such a platform than blocking compilation completely. Our test suite would still notice it.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile          |  1 +
 compat/nonblock.c | 23 +++++++++++++++++++++++
 compat/nonblock.h |  9 +++++++++
 3 files changed, 33 insertions(+)
 create mode 100644 compat/nonblock.c
 create mode 100644 compat/nonblock.h

Jeff King <peff@peff.net> writes:

Very well reasoned.

The only small "huh?" factor was that the name of the helper is not enable_nonblock(), but singles out pipe as valid thing to work on. I think it is perfectly fine, given that the current plan we have is to use this on the pipe with the command we spawn with pipe_command(), and it probably is even preferrable to explicitly declare that this is designed to only work with pipes and future developers who try to abuse it on other kinds of file descriptors are on their own. And "pipe" in the name of this helper may be such a declaration that is strong enough.

Thanks.

On Wed, Aug 17, 2022 at 01:23:25PM -0700, Junio C Hamano wrote:

My goal was to explain that "huh" with this bit in the commit message:

without going into too many details. I do think it would be easier to explain if the Windows implementation was added in the same commit (since it is explicitly just about pipes), but I wanted to credit René as the author there. I'm not sure if it's worth folding them together and adding a co-author credit instead. Or maybe I should state outright in this commit message that we're doing it for Windows. ;)

-Peff

From: René Scharfe <l.s.r@web.de>

Implement enable_pipe_nonblock() using the Windows API. This works only for pipes, but that is sufficient for this limited interface. Despite the API calls used, it handles both "named" and anonymous pipes from our pipe() emulation.

Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 compat/nonblock.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

We define MAX_IO_SIZE within wrapper.c, but it's useful for any code that wants to do a raw write() for whatever reason (say, because they want different EAGAIN handling). Let's make it available everywhere.

The alternative would be adding xwrite_foo() variants to give callers more options. But there's really no reason MAX_IO_SIZE needs to be abstracted away, so this give callers the most flexibility.

Signed-off-by: Jeff King <peff@peff.net>
---
I did actually write xwrite_nonblock() at first, and I could be
persuaded to go in that direction if we prefer. But given that using it
sanely requires the caller to have a poll() loop, I feel like
pipe_command() is already the right level of abstraction.

 git-compat-util.h | 22 ++++++++++++++++++++++
 wrapper.c         | 22 ----------------------
 2 files changed, 22 insertions(+), 22 deletions(-)

If xwrite() sees an EAGAIN response, it will loop forever until the write succeeds (or encounters a real error). This is due to ef1cf0167a (xwrite: poll on non-blocking FDs, 2016-06-26), with the idea that we won't be surprised by a descriptor unexpectedly set as non-blocking.

But that will make things awkward when we do want a non-blocking descriptor, and a future patch will switch pipe_command() to using one. In that case, looping on EAGAIN is bad, because the process on the other end of the pipe may be waiting on us before doing another read() on the pipe, which would mean we deadlock.

In practice we're not supposed to ever see EAGAIN here, since poll() will have just told us the descriptor is ready for writing. But our Windows emulation of poll() will always return "ready" for writing to a pipe descriptor! This is due to 94f4d01932 (mingw: workaround for hangs when sending STDIN, 2020-02-17).

Our best bet in that case is to keep handling other descriptors, as any read() we do may allow the child command to make forward progress (i.e., its write() finishes, and then it read()s from its stdin, freeing up space in the pipe buffer). This means we might busy-loop between poll() and write() on Windows if the child command is slow to read our input, but it's much better than the alternative of deadlocking.

In practice, this busy-looping should be rare:

  - for small inputs, we'll just write the whole thing in a single
    write() anyway, non-blocking or not

  - for larger inputs where the child reads input and then processes it
    before writing (e.g., gpg verifying a signature), we may make a few
    extra write() calls that get EAGAIN during the initial write, but
    once it has taken in the whole input, we'll correctly block waiting
    to read back the data.

  - for larger inputs where the child process is streaming output back
    (like a diff filter), we'll likewise see some extra EAGAINs, but
    most of them will be followed immediately by a read(), which will
    let the child command make forward progress.

Of course it won't happen at all for now, since we don't yet use a non-blocking pipe. This is just preparation for when we do.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

When write() to a non-blocking pipe fails because the buffer is full, POSIX says we should see EAGAIN. But our mingw_write() compat layer on Windows actually returns ENOSPC for this case. This is probably something we want to correct, but given that we don't plan to use non-blocking descriptors in a lot of places, we can work around it by just catching ENOSPC alongside EAGAIN. If we ever do fix mingw_write(), then this patch can be reverted.

We don't actually use a non-blocking pipe yet, so this is still just preparation.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
---
Ironically, this ENOSPC bug means that switching away from xwrite() in
the previous patch wasn't necessary (because it's not clever enough to
know that ENOSPC on a pipe means EAGAIN!). But I think handling both
shows the intent, and sets us up better for fixing mingw_write().

 run-command.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Jeff King <peff@peff.net> writes:

Yeah, I am impressed by the attention of small details by you two shown here to split the steps 4/6 and 5/6. If we consider that this step is a band-aid we'd be happier if we can remove, perhaps in-code comment to explain why we deal with ENOSPC here, instead of burying it only in the log message, would help remind people of the issue (but of course the patch is good with or without such a tweak, which is only relevant in the longer term).

On Wed, Aug 17, 2022 at 11:57:01AM -0700, Junio C Hamano wrote:

Yeah, you may be right. I had originally written a quite long comment here (before I split things up so much), but found in the splitting that it made more sense to put most of the details into the commit message.

But maybe it's worth squashing this in?

Our pipe_command() helper lets you both write to and read from a child process on its stdin/stdout. It's supposed to work without deadlocks because we use poll() to check when descriptors are ready for reading or writing. But there's a bug: if both the data to be written and the data to be read back exceed the pipe buffer, we'll deadlock.

The issue is that the code assumes that if you have, say, a 2MB buffer to write and poll() tells you that the pipe descriptor is ready for writing, that calling:

  write(cmd->in, buf, 2*1024*1024);

will do a partial write, filling the pipe buffer and then returning what it did write. And that is what it would do on a socket, but not for a pipe. When writing to a pipe, at least on Linux, it will block waiting for the child process to read() more. And now we have a potential deadlock, because the child may be writing back to us, waiting for us to read() ourselves.

An easy way to trigger this is:

  git -c add.interactive.useBuiltin=true \
      -c interactive.diffFilter=cat \
      checkout -p HEAD~200

The diff against HEAD~200 will be big, and the filter wants to write all of it back to us (obviously this is a dummy filter, but in the real world something like diff-highlight would similarly stream back a big output).

If you set add.interactive.useBuiltin to false, the problem goes away, because now we're not using pipe_command() anymore (instead, that part happens in perl). But this isn't a bug in the interactive code at all. It's the underlying pipe_command() code which is broken, and has been all along.

We presumably didn't notice because most calls only do input _or_ output, not both. And the few that do both, like gpg calls, may have large inputs or outputs, but never both at the same time (e.g., consider signing, which has a large payload but a small signature comes back).

The obvious fix is to put the descriptor into non-blocking mode, and indeed, that makes the problem go away. Callers shouldn't need to care, because they never see the descriptor (they hand us a buffer to feed into it).

The included test fails reliably on Linux without this patch. Curiously, it doesn't fail in our Windows CI environment, but has been reported to do so for individual developers. It should pass in any environment after this patch (courtesy of the compat/ layers added in the last few commits).

Signed-off-by: Jeff King <peff@peff.net>
---
Same as before, minus the compat/ bits which were already done, and we
can now claim victory on Windows, too.

 run-command.c              | 10 ++++++++++
 t/t3701-add-interactive.sh | 13 +++++++++++++
 2 files changed, 23 insertions(+)

On Wed, Aug 17, 2022 at 02:04:05AM -0400, Jeff King wrote:

The Windows CI did indeed complete successfully, though I'm not too surprised, since it never hit the blocking/deadlock case in the first place. I _think_ from our conversation that it should work for you, too, but please let me know. Fingers crossed. :)

-Peff

Am 17.08.2022 um 08:04 schrieb Jeff King:

Indeed: This series passes the test suite for me on Windows 11.

René

On Fri, Aug 19, 2022 at 11:19:08PM +0200, René Scharfe wrote:

Yay. :)

-Peff

Am 07.08.22 um 19:41 schrieb Jeff King:

You're right, Windows' write needs two corrections. The helper below reports what happens when we feed a pipe with writes of different sizes. On Debian on WSL 2 (Windows Subsystem for Linux) it says:

   chunk size: 1 bytes
   65536 total bytes written, then got EAGAIN
   chunk size: 1000 bytes
   64000 total bytes written, then got EAGAIN
   chunk size: 1024 bytes
   65536 total bytes written, then got EAGAIN
   chunk size: 100000 bytes
   0 total bytes written, then got a partial write of 65536 bytes
   65536 total bytes written, then got EAGAIN

On Windows directly I get:

   chunk size: 1 bytes
   8192 total bytes written, then got ENOSPC
   chunk size: 1000 bytes
   8000 total bytes written, then got ENOSPC
   chunk size: 1024 bytes
   8192 total bytes written, then got ENOSPC
   chunk size: 100000 bytes
   0 total bytes written, then got ENOSPC

https://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html documents what we should get: Writes smaller than the buffer should be atomic, bigger writes bigger should be broken up, and the error code for a full buffer should be EAGAIN. I.e. the first example is right. So mingw_write() needs to translate ENOSPC to EAGAIN and break up huge writes instead of giving up outright.

The two corrections mentioned above together with the enable_nonblock() implementation for Windows (and the removal of "false") suffice to let t3701 pass when started directly, but it still hangs when running the whole test suite using prove.

I don't have time to investigate right now, but I still don't understand how xwrite() can possibly work against a non-blocking pipe. It loops on EAGAIN, which is bad if the only way forward is to read from a different fd to allow the other process to drain the pipe buffer so that xwrite() can write again. I suspect pump_io_round() must not use xwrite() and should instead handle EAGAIN by skipping to the next fd.

René

---
 Makefile                 |  1 +
 t/helper/test-nonblock.c | 51 ++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c     |  1 +
 t/helper/test-tool.h     |  1 +
 4 files changed, 54 insertions(+)
 create mode 100644 t/helper/test-nonblock.c

On Wed, Aug 10, 2022 at 07:39:34AM +0200, René Scharfe wrote:

Thanks for digging into this further. What you found makes sense to me and explains what we're seeing.

Interesting. I wish there was an easy way for me to poke at this, too. I tried installing the Git for Windows SDK under wine, but unsurprisingly it did not get very far.

Possibly I could try connecting to a running CI instance, but the test did not seem to fail there! (Plus I'd have to figure out how to do that... ;) ).

Right, it's susceptible to looping forever in such a case. _But_ a blocking write is likewise susceptible to blocking forever. In either case, we're relying on the reading side to pull some bytes out of the pipe so we can make forward progress.

The key thing is that pump_io() is careful never to initiate a write() unless poll() has just told us that the descriptor is ready for writing.

If something unexpected happens there (i.e., the descriptor is not really ready), a blocking descriptor is going to be stuck. And with xwrite(), we're similarly stuck (just looping instead of blocking). Without xwrite(), a non-blocking one _could_ be better off, because that EAGAIN would make it up to pump_io(). But what is it supposed to do? I guess it could go back into its main loop and hope that whatever bug caused the mismatch between poll() and write() goes away.

But even that would not have fixed the problem here on Windows. From my understanding, mingw_write() in this case would never write _any_ bytes. So we'd never make forward progress, and just loop writing 0 bytes and returning EAGAIN over and over.

So I dunno. We could try to be a bit more defensive about non-blocking descriptors by avoiding xwrite() in this instance, but it only helps for a particular class of weird OS behavior/bugs. I'd prefer to see a real case that it would help before moving in that direction.

-Peff

Am 10.08.2022 um 21:53 schrieb Jeff King:

Right, and Windows breaks it by refusing to write data bigger than the buffer even if it's empty.

What does "ready for writing" mean?  PIPE_BUF bytes are free, right?

It should check other fds to let the other side make some progress on them, so that it eventually gets to drain the pipe we want to write to.

Right, we need to teach it to break up large writes. It must make at least some progress, if possible.

Makes sense.

René

On Thu, Aug 11, 2022 at 12:35:02AM +0200, René Scharfe wrote:

According to POSIX, it is "a write() will not block". ;)

I think in practice for Unix systems it means that at least 1 byte is available.

Yes, that "go back into its main loop" would do that. But that's not guaranteed to produce progress any time soon. We may just busy-loop between poll() and write() returning EAGAIN if another descriptor isn't ready. And if the thing preventing further writes is something the process on the other side of the pipe can't fix (and thus we in turn can't help it fix things by doing a read()), then we'll loop forever. Which (if I understand it) is exactly the case here because the underlying code refused to do a partial write.

-Peff

Am 05.08.2022 um 17:36 schrieb Jeff King:

Or how about this?  Squashable.  Needs testing.

--- >8 ---
Subject: [PATCH] nonblock: support Windows

Implement enable_nonblock() using the Windows API. Supports only named and anonymous pipes for now, which suffices for the current caller.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/nonblock.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Hi René,

On Sun, 7 Aug 2022, René Scharfe wrote:

Maybe use an `#elif defined(GIT_WINDOWS_NATIVE)` here? That would make the code structures clearer, methinks.

Nice.

FWIW the documentation of `PIPE_NOWAIT` says:

	Note that nonblocking mode is supported for compatibility with
	Microsoft LAN Manager version 2.0 and should not be used to
	achieve asynchronous input and output (I/O) with named pipes.

(see https://docs.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-setnamedpipehandlestate for full details)

There is little more information to be found on the interwebs, the closest to an in-depth explanation is here: https://devblogs.microsoft.com/oldnewthing/20110114-00/?p=11753

Even if that comment suggests that this mode is deprecated, I think it is safe to rely on it in Git's source code.

Thanks, Dscho

Hi Peff,

On Tue, 2 Aug 2022, Jeff King wrote:

It might be a bit heavy-handed to error out in this case, as it usually does not cause problems. At least that's what the fact suggests to me that I personally never encountered the dead-lock myself, and neither do I recall anybody piping more than two megabytes through `git checkout -p`.

Could we turn this into `warning_errno()` and avoid reporting an error here? We could add "; This might hang the process if large amounts of data are piped to/from <cmd>" to improve the user experience.

Ciao, Dscho

On Mon, Aug 08, 2022 at 02:59:49PM +0200, Johannes Schindelin wrote:

That thought crossed my mind, as well, but I'm hesitant to leave a known bug in place that can cause a deadlock. It would be one thing if we could muddle through without nonblock in a slower way, but I don't think we can easily detect this situation after the fact.

So maybe some options are:

  - don't bother with O_NONBLOCK unless the size of the input is over N
    bytes. The trouble there is that it's not clear what N should be.
    It's fcntl(F_GETPIPE_SZ) on Linux, but that's not portable. We could
    possibly come up with a conservative value if we had a ballpark for
    pipe size on Windows. It feels a bit hacky, though.

  - we could actually guess at a deadlock by putting a timeout on the
    poll(). That would also catch hanging or slow filter processes. I
    really hate putting clock-based limits on things, though, as it
    means the tool behaves differently under load. And keep in mind this
    is deep in the pipe_command() code. It happens to only trigger for
    diff filters now, but it may be used in other spots (in fact it
    already is, and it's only the size of current gpg payloads/responses
    that means it doesn't happen to trigger).

Stepping back, though, I think we should consider why we'd see an error here. I wouldn't expect it to ever fail on a system where O_NONBLOCK was supported. If we want to make it a silent noop on some platforms, then we can stick that into the enable_nonblock() function (which is what I did, but as René showed, that is probably not a good enough solution).

-Peff

Hi Peff,

On Tue, 9 Aug 2022, Jeff King wrote:

Ugh, I think that my reasoning was flawed, as I somehow based it on the assumption that `enable_nonblock()` would return -1 on platforms without O_NONBLOCK. Even if I had read that you fall back to returning 0 on those platforms.

And only when reading your reply did it occur to me that this was a thinko on my part.

So I would like to retract my assessment that it is heavy-handed to error out in this case. It would have been if we had errored out on platforms without O_NONBLOCK support, but we don't.

Sorry for the noise, Dscho