Re: [PATCH v6 1/2] run-command: add an "ungroup" option to run_process_parallel()

From: Emily Shaffer <emilyshaffer@google.com>
Date: Jun 17, 2022, 00:07 UTC
Message-ID: <YqvFpcfUqyL+SlhB@google.com>
In-Reply-To: <patch-v6-1.2-45248c786d7-20220606T170356Z-avarab@gmail.com>

On Tue, Jun 07, 2022 at 10:48:19AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 11 quoted lines

> @@ -1766,8 +1791,15 @@ int run_processes_parallel(int n,
>  		}
>  		if (!pp.nr_processes)
>  			break;
> -		pp_buffer_stderr(&pp, output_timeout);
> -		pp_output(&pp);
> +		if (ungroup) {
> +			int i;
> +
> +			for (i = 0; i < pp.max_processes; i++)
> +				pp.children[i].state = GIT_CP_WAIT_CLEANUP;

FYI, this broke for us downstream where we are carrying patches adding a 'pp_buffer_stdin()' and friends to enable stdin buffering to parallel processes. It also appears to break when pp.max_processes exceeds the number of actual tasks provided. I needed to add something like this

+    if (pp.children[i].state == GIT_CP_WORKING &&
+        !pp.children[i].process.in)
+            pp.children[i].state = GIT_CP_WAIT_CLEANUP;

That is, only set the WAIT_CLEANUP state if the task wasn't waiting to be given work (GIT_CP_FREE -> GIT_CP_WAIT_CLEANUP leads to some "error: waitpid is confused" errors) and if stdin is not currently being buffered. In the case where .process.in is > 0, GIT_CP_WAIT_CLEANUP causes pp_collect_finished() to stop spinning in this IO buffer loop, but there is still stdin to pass along.

Anyway, I think the first part (GIT_CP_FREE -> GIT_CP_WAIT_CLEANUP) is a bug that matters for the patch as it is now; the stdin bit will not matter until the later config-based-hooks patches which introduce stdin buffering anyway.

Thanks very much for this fix otherwise.

 - Emily

Show 7 quoted lines

> +		} else {
> +			pp_buffer_stderr(&pp, output_timeout);
> +			pp_output(&pp);
> +		}
>  		code = pp_collect_finished(&pp);
>  		if (code) {
>  			pp.shutdown = 1;