Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid

[Johannes Sixt <j6t@kdbg.org>]

Am 07.11.2015 um 00:48 schrieb Stefan Beller:
> Detect if a child stopped working by checking if their stderr pipe
> was closed instead of checking their state with waitpid.
> As waitpid is not fully working in Windows, this is an approach which
> allows for better cross platform operation. (It's less code, too)
>
> Previously we did not close the read pipe of finished children, which we
> do now.
>
> The old way missed some messages on an early abort. We just killed the
> children and did not bother to look what was left over. With this approach
> we'd send a signal to the children and wait for them to close the pipe to
> have all the messages (including possible "killed by signal 15" messages).
>
> To have the test suite passing as before, we allow for real graceful
> abortion now. In case the user wishes to abort parallel execution
> the user needs to provide either the signal used to kill all children
> or the children are let run until they finish normally.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>   Hi,
>
>   this applis on top of origin/sb/submodule-parallel-fetch,
>   making Windows folks possibly even more happy. It makes the code easier
>   to read and has less races on cleaning up a terminated child.
>
>   It follows the idea of Johannes patch, instead of encoding information in .err
>   I removed the in_use flag and added a state, currently having 3 states.
>
>   Thanks,
>   Stefan
>
>   Johannes schrieb:
>   > First let me say that I find it very questionable that the callbacks
>   > receive a struct child_process.
>
>   I tried to get rid of the child_process struct in the callbacks, but that's
>   not as easy as one may think.

Fair enough. I see you removed .err, .no_stdin and .stdout_to_stderr 
from the callback. Good.

>   		pp->nr_processes--;
> -		pp->children[i].in_use = 0;
> +		pp->children[i].state = FREE;
>   		pp->pfd[i].fd = -1;
>   		child_process_deinit(&pp->children[i].process);

This cleanup is implied by finish_command and can be removed.

>   		child_process_init(&pp->children[i].process);

> @@ -1195,12 +1175,12 @@ int run_processes_parallel(int n,
>   		    i < spawn_cap && !pp->shutdown &&
>   		    pp->nr_processes < pp->max_processes;
>   		    i++) {
> -			int code = pp_start_one(pp);
> +			code = pp_start_one(pp);
>   			if (!code)
>   				continue;
>   			if (code < 0) {
>   				pp->shutdown = 1;
> -				kill_children(pp, SIGTERM);
> +				kill_children(pp, -code);

I'll see what this means for our kill emulation on Windows. Currently, 
we handle only SIGTERM.

>   			}
>   			break;
>   		}

Thanks you very much!

-- Hannes