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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 14, 2024, 17:09 UTC
Message-ID: <xmqqa5gatb4v.fsf@gitster.g>
In-Reply-To: <20240914064130.GA1284567@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 8 quoted lines

> 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.

Show 14 quoted lines

> +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.

Show 9 quoted lines

> @@ -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".

Show 6 quoted lines

>  	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.

Show 20 quoted lines

> 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.