git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Git mailing list" <git@vger.kernel.org>,
	"Clemens Buchacher" <drizzd@gmx.net>
Subject: [PATCH 2/2] fetch: ignore SIGPIPE during network operation
Date: Sun, 3 Mar 2019 11:58:43 -0500	[thread overview]
Message-ID: <20190303165843.GB23755@sigill.intra.peff.net> (raw)
In-Reply-To: <20190303165447.GA31116@sigill.intra.peff.net>

The default SIGPIPE behavior can be useful for a command that generates
a lot of output: if the receiver of our output goes away, we'll be
notified asynchronously to stop generating it (typically by killing the
program).

But for a command like fetch, which is primarily concerned with
receiving data and writing it to disk, an unexpected SIGPIPE can be
awkward. We're already checking the return value of all of our write()
calls, and dying due to the signal takes away our chance to gracefully
handle the error.

On Linux, we wouldn't generally see SIGPIPE at all during fetch. If the
other side of the network connection hangs up, we'll see ECONNRESET. But
on OS X, we get a SIGPIPE, and the process is killed. This causes t5570
to racily fail, as we sometimes die by signal (instead of the expected
die() call) when the server side hangs up.

Let's ignore SIGPIPE during the network portion of the fetch, which will
cause our write() to return EPIPE, giving us consistent behavior across
platforms.

This fixes the test flakiness, but note that it stops short of fixing
the larger problem. The server side hit a fatal error, sent us an "ERR"
packet, and then hung up. We notice the failure because we're trying to
write to a closed socket. But by dying immediately, we never actually
read the ERR packet and report its content to the user. This is a (racy)
problem on all platforms. So this patch lays the groundwork from which
that problem might be fixed consistently, but it doesn't actually fix
it.

Note the placement of the SIGPIPE handling. The absolute minimal change
would be to ignore SIGPIPE only when we're writing. But twiddling the
signal handler for each write call is inefficient and maintenance
burden. On the opposite end of the spectrum, we could simply declare
that fetch does not need SIGPIPE handling, since it doesn't generate a
lot of output, and we could just ignore it at the start of cmd_fetch().

This patch takes a middle ground. It ignores SIGPIPE during the network
operation (which is admittedly most of the program, since the actual
network operations are all done under the hood by the transport code).
So it's still pretty coarse.

Signed-off-by: Jeff King <peff@peff.net>
---
I had a hard time trying to write the last two paragraphs. I think the
placement here really is not significantly different than just ignoring
SIGPIPE for the entirety of cmd_fetch(). I kind of like your suggestion
elsewhere in the thread to just teach the git wrapper an IGNORE_SIGPIPE
flag.

 builtin/fetch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b620fd54b4..4ba63d5ac6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1556,7 +1556,9 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru
 
 	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack);
+	sigchain_push(SIGPIPE, SIG_IGN);
 	exit_code = do_fetch(gtransport, &rs);
+	sigchain_pop(SIGPIPE);
 	refspec_clear(&rs);
 	transport_disconnect(gtransport);
 	gtransport = NULL;
-- 
2.21.0.675.g01c085a870

  parent reply	other threads:[~2019-03-03 16:58 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-06 15:11 t5570-git-daemon fails with SIGPIPE on OSX SZEDER Gábor
2018-08-06 15:31 ` SZEDER Gábor
2019-02-08  8:32   ` Johannes Schindelin
2019-02-08 12:54     ` SZEDER Gábor
2018-08-14 22:32 ` Jeff King
2018-08-14 22:37   ` Jeff King
2019-02-08  9:02   ` Johannes Schindelin
2019-02-08  9:28     ` Johannes Schindelin
2019-02-08 19:54       ` Jeff King
2019-03-01 15:02         ` Johannes Schindelin
2019-03-01 19:00           ` Jeff King
2019-03-02 21:21             ` Johannes Schindelin
2019-03-03 16:54               ` Jeff King
2019-03-03 16:55                 ` [PATCH 1/2] fetch: avoid calling write_or_die() Jeff King
2019-03-04 13:42                   ` Duy Nguyen
2019-03-05  4:11                     ` Jeff King
2019-03-03 16:58                 ` Jeff King [this message]
2019-03-04  1:11                   ` [PATCH 2/2] fetch: ignore SIGPIPE during network operation Junio C Hamano
2019-03-05  4:11                     ` Jeff King
2019-03-03  1:21             ` t5570-git-daemon fails with SIGPIPE on OSX Junio C Hamano
2019-03-03 14:56               ` 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=20190303165843.GB23755@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=drizzd@gmx.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=szeder.dev@gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).