Re: [PATCH 2/4] transport-helper: check if remote helper is alive

[Jeff King <peff@peff.net>]

On Mon, Apr 01, 2013 at 03:46:42PM -0600, Felipe Contreras wrote:

> Otherwise transport-helper will continue checking for refs and other
> things what will confuse the user more.
> [...]
> diff --git a/transport-helper.c b/transport-helper.c
> index cb3ef7d..dfdfa7a 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport,
>  
>  	if (finish_command(&fastimport))
>  		die("Error while running fast-import");
> +
> +	if (!check_command(data->helper))
> +		die("Error while running helper");
> +
>  	argv_array_free_detached(fastimport.argv);

Can you be more specific about what happens when we miss the death here,
what happens next, etc?

Checking asynchronously for death like this is subject to a race
condition; the helper may be about to die but not have died yet. In
practice we may catch some cases, but this seems like an indication that
the protocol is not well thought-out. Usually we would wait for a
confirmation over the read pipe from a child, and know that the child
failed when either:

  1. It tells us so on the pipe.

  2. The pipe closes (at which point we know it is time to reap the
     child).

Why doesn't that scheme work here? I am not doubting you that it does
not; the import helper protocol is a bit of a mess, and I can easily
believe it has such a problem. But I'm wondering if it's possible to
improve it in a more robust way.

-Peff