Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 9, 2022, 22:10 UTC
- Message-ID
- <4o5p308o-9r66-2853-qon0-r1464p2o83qn@tzk.qr>
- In-Reply-To
- <YvJbXJyaKz5QPYdz@coredump.intra.peff.net>
Hi Peff,
On Tue, 9 Aug 2022, Jeff King wrote:
Show 19 quoted lines
> On Mon, Aug 08, 2022 at 02:59:49PM +0200, Johannes Schindelin wrote:
>
> > 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`.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
Show 31 quoted lines
> > 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 >