Re: Help needed on 2.54.0-rc0 t5301.13 looping.

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

Jeff King <peff@peff.net> writes:

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