From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 02 Oct 2025 06:34:18 GMT
Subject: Re: [PATCH 01/10] run-command: add stdin callback for parallelization
Message-ID: <aN4c6l7gRi4auss1@pks.im>
In-Reply-To: <20250925125352.1728840-2-adrian.ratiu@collabora.com>

On Thu, Sep 25, 2025 at 03:53:44PM +0300, Adrian Ratiu wrote:
> diff --git a/run-command.c b/run-command.c
> index ed9575bd6a..6c455a0e43 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1652,6 +1652,44 @@ static int pp_start_one(struct parallel_processes *pp,
>  	return 0;
>  }
>  
> +static void pp_buffer_stdin(struct parallel_processes *pp,
> +			    const struct run_process_parallel_opts *opts)
> +{
> +	/* Buffer stdin for each pipe. */
> +	for (int i = 0; i < opts->processes; i++) {

`opts->processes` is of type `size_t`, so let's use the same type as
iterator.

> +		struct child_process *proc = &pp->children[i].process;
> +		int ret;
> +
> +		if (pp->children[i].state != GIT_CP_WORKING || proc->in <= 0)
> +			continue;
> +
> +		/**

Nit: multi-line comments should start with "/*", not "/**". This is also
present in multiple other 

> +		 * child input is provided via path_to_stdin when the feed_pipe cb is
> +		 * missing, so we just signal an EOF.
> +		 */
> +		if (!opts->feed_pipe) {
> +			close(proc->in);
> +			proc->in = 0;

Hm. It's curious that we use a valid file descriptor here. Shouldn't we
rather use `-1`? Otherwise I could see that we might try to close this
seemingly valid file descriptor at a later point in time.

> +			continue;
> +		}
> +
> +		/**
> +		 * Feed the pipe:
> +		 *   ret < 0 means error
> +		 *   ret == 0 means there is more data to be fed
> +		 *   ret > 0 means feeding finished
> +		 */
> +		ret = opts->feed_pipe(proc->in, opts->data, pp->children[i].data);
> +		if (ret < 0)
> +			die_errno("feed_pipe");
> +
> +		if (ret == 1) {

This condition mismatches the comment: you explicitly check for 1, but
the comment above says `ret > 0` indicates that feeding has finished.

> @@ -1756,6 +1795,33 @@ static int pp_collect_finished(struct parallel_processes *pp,
>  	return result;
>  }
>  
> +static void pp_handle_child_IO(struct parallel_processes *pp,
> +				const struct run_process_parallel_opts *opts,
> +				int output_timeout)

Okay, this function is new and was extracted out of
`run_processes_parallel()`. It's basically the heart of our I/O loop
for our children.

> +{
> +	/*
> +	 * First push input, if any (it might no-op), to child tasks to avoid them blocking
> +	 * after input. This also prevents deadlocks when ungrouping below, if a child blocks
> +	 * while the parent also waits for them to finish.
> +	 */
> +	pp_buffer_stdin(pp, opts);

This part is new, as we now know to also optionally write stdin to the
child process.

> +	if (opts->ungroup) {
> +		for (size_t i = 0; i < opts->processes; i++) {
> +			int child_ready_for_cleanup =
> +				pp->children[i].state == GIT_CP_WORKING &&
> +				pp->children[i].process.in == 0;
> +
> +			if (child_ready_for_cleanup)
> +				pp->children[i].state = GIT_CP_WAIT_CLEANUP;

And this part here has changed, as well. We don't unconditionally set
`GIT_CP_WAIT_CLEANUP` anymore, but wait for `process.in` to be closed.

> +		}
> +		return;

I feel like this return is easy to miss. I think an `else` branch would
be more obvious.

> @@ -1775,6 +1841,13 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
>  					   "max:%"PRIuMAX,
>  					   (uintmax_t)opts->processes);
>  
> +	/*
> +	 * 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.
> +	 */
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
>  	pp_init(&pp, opts, &pp_sig);
>  	while (1) {
>  		for (i = 0;

Yeah, makes sense. I was briefly wondering whether we should rather do
it as part of `pp_buffer_stdin()`, so that it's more contained. But I'm
not sure that buys us anything.

> @@ -1809,8 +1876,11 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
>  
>  	pp_cleanup(&pp, opts);
>  
> +	sigchain_pop(SIGPIPE);
> +

There are no early exits, so we know this code should be executed.

>  	if (do_trace2)
>  		trace2_region_leave(tr2_category, tr2_label, NULL);
> +
>  }
>  
>  int prepare_auto_maintenance(int quiet, struct child_process *maint)

Nit: stray empty line.

> diff --git a/run-command.h b/run-command.h
> index 0df25e445f..4679987c8e 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -420,6 +420,22 @@ typedef int (*start_failure_fn)(struct strbuf *out,
>  				void *pp_cb,
>  				void *pp_task_cb);
>  
> +/**
> + * This callback is repeatedly called on every child process who requests
> + * start_command() to create a pipe by setting child_process.in < 0.
> + *
> + * pp_cb is the callback cookie as passed into run_processes_parallel, and
> + * pp_task_cb is the callback cookie as passed into get_next_task_fn.
> + * The contents of 'send' will be read into the pipe and passed to the pipe.
> + *
> + * Returns < 0 for error
> + * Returns == 0 when there is more data to be fed (will be called again)
> + * Returns > 0 when finished (child closes fd or no more data to be fed)

s/closes/closed/

> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 3719f23cc2..dfdb03b3ab 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c

This helper very much looks like it should be converted to a unit test.
Anyway, that is outside of the scope of this patch series.

> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 76d4936a87..282afecefc 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -164,6 +164,36 @@ test_expect_success 'run_command runs ungrouped in parallel with more tasks than
>  	test_line_count = 4 err
>  '
>  
> +cat >expect <<-EOF
> +preloaded output of a child
> +listening for stdin:
> +sample stdin 1
> +sample stdin 0
> +preloaded output of a child
> +listening for stdin:
> +sample stdin 1
> +sample stdin 0
> +preloaded output of a child
> +listening for stdin:
> +sample stdin 1
> +sample stdin 0
> +preloaded output of a child
> +listening for stdin:
> +sample stdin 1
> +sample stdin 0
> +EOF

This block should be part of the test itself.

> +test_expect_success 'run_command listens to stdin' '
> +	write_script stdin-script <<-\EOF &&
> +	echo "listening for stdin:"
> +	while read line; do

Style nit: let's drop the `;` and move the `do` to the next line.

> +		echo "$line"
> +	done
> +	EOF
> +	test-tool run-command run-command-stdin 2 ./stdin-script 2>actual &&
> +	test_cmp expect actual
> +'

Patrick