Re: [PATCH 01/10] run-command: add stdin callback for parallelization
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 2, 2025, 06:34 UTC
- 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:
Show 13 quoted lines
> 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.
Show 7 quoted lines
> + 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
Show 6 quoted lines
> + * 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.
Show 14 quoted lines
> + 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.
Show 7 quoted lines
> @@ -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.
Show 7 quoted lines
> +{
> + /*
> + * 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.
Show 8 quoted lines
> + 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.
Show 14 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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.
Show 6 quoted lines
> 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.
Show 19 quoted lines
> 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.
Show 26 quoted lines
> 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.
Show 6 quoted lines
> + echo "$line" > + done > + EOF > + test-tool run-command run-command-stdin 2 ./stdin-script 2>actual && > + test_cmp expect actual > +'
Patrick