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

[Junio C Hamano <gitster@pobox.com>]

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.