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

From: Jeff King <peff@peff.net>
Date: Aug 9, 2022, 13:04 UTC
Message-ID: <YvJbXJyaKz5QPYdz@coredump.intra.peff.net>
In-Reply-To: <q341oso8-1ps6-65n6-s394-n8q433q79nr2@tzk.qr>

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

Show 17 quoted lines

> On Tue, 2 Aug 2022, Jeff King wrote:
> 
> > diff --git a/run-command.c b/run-command.c
> > index 14f17830f5..ed99503b22 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1438,6 +1439,15 @@ int pipe_command(struct child_process *cmd,
> >  		return -1;
> >
> >  	if (in) {
> > +		if (enable_nonblock(cmd->in) < 0) {
> > +			error_errno("unable to make pipe non-blocking");
> 
> 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`.

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