On Sat, Sep 14, 2024 at 10:09:52AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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
