From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking
Date: Tue, 9 Aug 2022 09:04:28 -0400 [thread overview]
Message-ID: <YvJbXJyaKz5QPYdz@coredump.intra.peff.net> (raw)
In-Reply-To: <q341oso8-1ps6-65n6-s394-n8q433q79nr2@tzk.qr>
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`.
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
next prev parent reply other threads:[~2022-08-09 13:04 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-02 4:13 [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking Jeff King
2022-08-02 15:04 ` Junio C Hamano
2022-08-02 15:39 ` Jeff King
2022-08-02 16:16 ` Junio C Hamano
2022-08-03 3:53 ` [PATCH v2] " Jeff King
2022-08-03 16:45 ` René Scharfe
2022-08-03 17:20 ` Jeff King
2022-08-03 21:56 ` René Scharfe
2022-08-05 15:36 ` Jeff King
2022-08-05 21:13 ` René Scharfe
2022-08-07 10:15 ` René Scharfe
2022-08-07 17:41 ` Jeff King
2022-08-10 5:39 ` René Scharfe
2022-08-10 19:53 ` Jeff King
2022-08-10 22:35 ` René Scharfe
2022-08-11 8:52 ` Jeff King
2022-08-10 5:39 ` [PATCH] mingw: handle writes to non-blocking pipe René Scharfe
2022-08-10 9:07 ` Johannes Schindelin
2022-08-10 20:02 ` Jeff King
2022-08-10 22:34 ` René Scharfe
2022-08-11 8:47 ` Jeff King
2022-08-11 17:35 ` René Scharfe
2022-08-11 18:20 ` Jeff King
2022-08-14 15:37 ` René Scharfe
2022-08-17 5:39 ` Jeff King
2022-08-17 6:04 ` [PATCH v2 0/6] fix pipe_command() deadlock Jeff King
2022-08-17 6:04 ` [PATCH v2 1/6] compat: add function to enable nonblocking pipes Jeff King
2022-08-17 20:23 ` Junio C Hamano
2022-08-18 5:41 ` Jeff King
2022-08-17 6:05 ` [PATCH v2 2/6] nonblock: support Windows Jeff King
2022-08-17 6:06 ` [PATCH v2 3/6] git-compat-util: make MAX_IO_SIZE define globally available Jeff King
2022-08-17 6:08 ` [PATCH v2 4/6] pipe_command(): avoid xwrite() for writing to pipe Jeff King
2022-08-17 6:09 ` [PATCH v2 5/6] pipe_command(): handle ENOSPC when writing to a pipe Jeff King
2022-08-17 18:57 ` Junio C Hamano
2022-08-18 5:38 ` Jeff King
2022-08-17 6:10 ` [PATCH v2 6/6] pipe_command(): mark stdin descriptor as non-blocking Jeff King
2022-08-17 6:20 ` [PATCH v2 0/6] fix pipe_command() deadlock Jeff King
2022-08-19 21:19 ` René Scharfe
2022-08-20 7:04 ` Jeff King
2022-08-07 10:14 ` [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking René Scharfe
2022-08-08 12:55 ` Johannes Schindelin
2022-08-08 12:59 ` Johannes Schindelin
2022-08-09 13:04 ` Jeff King [this message]
2022-08-09 22:10 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YvJbXJyaKz5QPYdz@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.