[PATCH v2 0/6] fix pipe_command() deadlock

From: Jeff King <peff@peff.net>
Date: Aug 17, 2022, 06:04 UTC
Message-ID: <YvyE1RDFVAg4uQo1@coredump.intra.peff.net>
In-Reply-To: <Yvx/FNlTx+DCiCOj@coredump.intra.peff.net>

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

> 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.

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