Re: Help needed on 2.54.0-rc0 t5301.13 looping.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 8, 2026, 15:50 UTC
- Message-ID
- <xmqqpl491mfd.fsf@gitster.g>
- In-Reply-To
- <20260408052031.GB1324339@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 43 quoted lines
> I think the root of the issue is that we should not be trying to
> propagate SIGPIPE to the child in this case at all. Our handler is
> pushed there only because it's part of sigchain_push_common(), which is
> sensible: in general if we are dying to SIGPIPE we want to do our
> cleanup. It's just funny in this case with the ordering of our SIG_IGN,
> because now that SIG_IGN isn't on top of the stack anymore.
>
> I.e., I think we want to reorder like this:
>
> diff --git a/run-command.c b/run-command.c
> index 32c290ee6a..8a95f7ff1e 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1895,14 +1895,19 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
> "max:%"PRIuMAX,
> (uintmax_t)opts->processes);
>
> + pp_init(&pp, opts, &pp_sig);
> +
> /*
> * Child tasks might receive input via stdin, terminating early (or not), so
> * ignore the default SIGPIPE which gets handled by each feed_pipe_fn which
> * actually writes the data to children stdin fds.
> + *
> + * This _must_ come after pp_init(), because it installs its own
> + * SIGPIPE handler (to cleanup children), and we want to supersede
> + * that.
> */
> sigchain_push(SIGPIPE, SIG_IGN);
>
> - pp_init(&pp, opts, &pp_sig);
> while (1) {
> for (i = 0;
> i < spawn_cap && !pp.shutdown &&
>
> Does that make your problem go away?
>
> I suspect we could construct a related case that does fail on Linux
> without the patch above. Imagine we actually have two hooks running in
> parallel. The first one is fast and does not read its input, and the
> second one is slow. We'll get SIGPIPE writing to the first one, and then
> kill _both_ children. But that's wrong! There is no reason to kill the
> second hook, as our intent was to ignore SIGPIPE.Oh, I am very much impressed by this analysis.
As -rc1 has already been tagged (but not pushed out yet), we would probably want to apply a fix before -rc2, I suppose.
Thanks.