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

Message

Jeff King Aug. 17, 2022, 6:04 a.m. UTC

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

Comments

Jeff King Aug. 17, 2022, 6:20 a.m. UTC | #1

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

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

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

René Scharfe Aug. 19, 2022, 9:19 p.m. UTC | #2

Am 17.08.2022 um 08:04 schrieb Jeff King:
> 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).

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

René

Jeff King Aug. 20, 2022, 7:04 a.m. UTC | #3

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

> Am 17.08.2022 um 08:04 schrieb Jeff King:
> > 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).
> 
> Indeed: This series passes the test suite for me on Windows 11.

Yay. :)

-Peff