Re: [PATCH] t5512.40 sometimes dies by SIGPIPE

[Patrick Steinhardt <ps@pks.im>]

On Sat, Sep 14, 2024 at 10:09:52AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > I really wish there was a way to ignore SIGPIPE per-descriptor (or even
> > tell which descriptor triggered it in a signal handler), but I don't
> > think there is.
> > ...
> > Another option is to selectively disable/re-enable SIGPIPE for
> > individual write() calls. That's not thread-safe, and I imagine may have
> > some performance penalty in practice due to the extra syscalls. But it
> > might make sense to do it selectively.
> 
> All true.
> 
> > +static int write_constant_gently(int fd, const char *str)
> >  {
> >  	if (debug)
> >  		fprintf(stderr, "Debug: Remote helper: -> %s", str);
> >  	if (write_in_full(fd, str, strlen(str)) < 0)
> > +		return -1;
> > +	return 0;
> > +}
> > +
> > +static void write_constant(int fd, const char *str)
> > +{
> > +	if (write_constant_gently(fd, str) < 0)
> >  		die_errno(_("full write to remote helper failed"));
> >  }
> 
> The _gently variant is meant to be (optionally) used while SIGPIPE
> is ignored, and this one is meant to be always used while SIGPIPE is
> not.  If the reading end closed the fd, the underlying write would
> fail and trigger SIGPIPE.  This die_errno() will not trigger in such
> a case.  But for other kind of errors, we die just like we used to
> in the original code.  OK.
> 
> > @@ -168,13 +175,16 @@ static struct child_process *get_helper(struct transport *transport)
> >  		die_errno(_("can't dup helper output fd"));
> >  	data->out = xfdopen(duped, "r");
> >  
> > -	write_constant(helper->in, "capabilities\n");
> > +	sigchain_push(SIGPIPE, SIG_IGN);
> > +	if (write_constant_gently(helper->in, "capabilities\n") < 0)
> > +		die("remote helper '%s' aborted session", data->name);
> > +	sigchain_pop(SIGPIPE);
> 
> And the only caller of the _gently variant with SIGPIPE ignored
> supplies its own error message.  It is easier for the end-user to
> identify than a generic "Broken pipe".  Nice.
> 
> We identified a helper that closes the connection even before it
> hears from us, so we say "aborted".
> 
> >  	while (1) {
> >  		const char *capname, *arg;
> >  		int mandatory = 0;
> >  		if (recvline(data, &buf))
> > -			exit(128);
> > +			die("remote helper '%s' aborted session", data->name);
> 
> Here, we found a helper that failed to talk back to us (they may
> have actually read what we wrote initially, or what we wrote may be
> hanging in the pipe buffer without being read).  It depends on the
> timing between which of the above two points we detect such a
> helper, so using the same error message does make sense.
> 
> > diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
> > index 20f43f7b7d..d21877150e 100755
> > --- a/t/t5801-remote-helpers.sh
> > +++ b/t/t5801-remote-helpers.sh
> > @@ -344,4 +344,15 @@ test_expect_success 'fetch tag' '
> >  	compare_refs local v1.0 server v1.0
> >  '
> >  
> > +test_expect_success 'totally broken helper reports failure message' '
> > +	write_script git-remote-broken <<-\EOF &&
> > +	read cap_cmd
> > +	exit 1
> > +	EOF
> > +	test_must_fail \
> > +		env PATH="$PWD:$PATH" \
> > +		git clone broken://example.com/foo.git 2>stderr &&
> > +	grep aborted stderr
> > +'
> > +
> >  test_done
> 
> The code change covers both a helper that disconnects before we
> write the first greeting and a helper that disconnects while we are
> still expecting it to talk to us.  The test explicitly covers the
> latter by reading our greeting (in other words, it does not die
> before we do our initial "write"---no risk of the SIGPIPE) and then
> stopping without writing anything.  As we are waiting to read from
> the helper, we will see an error in recvline().
> 
> Nice.
> 
> If the test loses the initial "read the greeting", then some of the
> time our greeting would be hanging in the pipe, we wait in read, and
> notice that the helper died, to trigger the "recvline() failed" code
> path.  But other times, the helper would die even before we write
> the greeting, so we'd see an error from write_constant_gently() and
> die with an identical message.  Such a test won't suffer from any
> flakyness but makes me feel somewhat dirty, so the above is good
> ;-).

Thanks to both of you for catching and addressing this issue! Both
patches look sensible to me.

Patrick