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: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Git mailing list" <git@vger.kernel.org>,
	"Clemens Buchacher" <drizzd@gmx.net>
Subject: Re: t5570-git-daemon fails with SIGPIPE on OSX
Date: Fri, 1 Mar 2019 14:00:42 -0500	[thread overview]
Message-ID: <20190301190042.GF30847@sigill.intra.peff.net> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1903011557430.41@tvgsbejvaqbjf.bet>

On Fri, Mar 01, 2019 at 04:02:22PM +0100, Johannes Schindelin wrote:

> > I think that patch does the write_or_die conversion to handle EPIPE, but
> > it would still need to turn off SIGPIPE for the whole process.
> > 
> > So you'd also need to stick a:
> > 
> >   sigchain_push(SIGPIPE, SIG_IGN);
> > 
> > somewhere near the start of cmd_fetch(). (There may be a less
> > coarse-grained place to put it, but at this point I think we're just
> > trying to find out whether this approach even solves the problem).
> 
> This fixes it, it seems. I let the job run with `--stress=50` and even
> after half an hour, it did not fail:
> 
> attempts ://git.visualstudio.com/git/_build/results?buildId=354

Cool, I'm glad it works!

> (I had to cancel it, I thought that `--stress=50` would stop trying after
> 50 runs, but I was obviously incorrect in that assumption...)

No, that will do 50 simultaneous jobs. You want --stress-limit=50.

It might make more sense to have --stress-jobs, and make --stress=X set
the limit, as I think it's much more useful to set the limit than it is
to set the number of jobs. At least that's my experience.

> So. Good, we have a diff that works. But you mentioned that you'd like to
> put it somewhere else? I am a bit unfamiliar with the code paths of
> `cmd_fetch()`, so I would be hard pressed to find a better spot. Any hint?

Well, I'm of two minds there. If we want to make the minimum change,
then we'd just want to disable SIGPIPE while we're actually conversing
with the other side. So I guess that would be somewhere in do_fetch(),
before we start talking to the other side, and restoring it after we've
finished our half of the conversation (i.e., after we've done all the
want/have bits and we're receiving the pack). But that's actually pretty
awkward to do, because most of those details are handled under the hood
by the transport code.

So probably something like this is the least-terrible place to put it:

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;

That said, I actually think it's kind of pointless for git-fetch to use
SIGPIPE at all. The purpose of SIGPIPE is that you can write a naive
program that spews output, and you'll get informed (forcefully) by the
OS if the process consuming your output stops listening. That makes
sense for programs like "git log", whose primary purpose is generating
output.

But for git-fetch, our primary purpose is receiving data and writing it
to disk. We should be checking all of our write()s already, and SIGPIPE
is just confusing. The only "big" output we generate is the status table
at the end. And even if that is going to a pipe that closes, I don't
think we'd want to fail the whole command (we'd want to finalize any
writes for what we just fetched, clean up after ourselves, etc).

So I'd actually be fine with just declaring that certain commands (like
fetch) just ignore SIGPIPE entirely.

-Peff

  reply	other threads:[~2019-03-01 19:00 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 [this message]
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                 ` [PATCH 2/2] fetch: ignore SIGPIPE during network operation Jeff King
2019-03-04  1:11                   ` 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=20190301190042.GF30847@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=drizzd@gmx.net \
    --cc=git@vger.kernel.org \
    --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).