Re: [PATCH v3] receive-pack: check if client is alive before completing the push

[Junio C Hamano <gitster@pobox.com>]

Robin Jarry <robin.jarry@6wind.com> writes:

> Abort the push operation (i.e. do not migrate the objects from temporary
> to permanent storage) if the client has disconnected while the
> pre-receive hook was running.
>
> This reduces the risk of inconsistencies on network errors or if the
> user hits ctrl-c while the pre-receive hook is running.
>
> Send a keepalive packet (empty) on sideband 2 (the one to report
> progress). If the client has exited, receive-pack will be killed via
> SIGPIPE and the push will be aborted. This only works when sideband*
> capabilities are advertised by the client.

If they have already exited but the fact hasn't reached us over the
network, the write() will succeed to deposit the packet in the send
buffer.  So I am not sure how much this would actually help, but it
should be safe to send an unsolicited keepalive as long as the other
side is expecting to hear from us.  When either report_status or
report_status_v2 capabilities is in effect, we will make a report()
or report_v2() call later, so we should be safe.

> Signed-off-by: Robin Jarry <robin.jarry@6wind.com>
> ---
> v2 -> v3:
>     I had missed Documentation/technical/pack-protocol.txt. Using
>     sideband 2 to send the keepalive packet works.
>
>  builtin/receive-pack.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9f4a0b816cf9..8b0d56897c9f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands,
>  		return;
>  	}
>  
> +	/*
> +	 * Send a keepalive packet on sideband 2 (progress info) to ensure that
> +	 * the client has not disconnected while pre-receive was running.
> +	 */

I suspect that any keepalive, unless it expects an active "yes, I am
still alive" response from the other side, is too weak to "ensure".

I guess "to notice a client that has disconnected (e.g. killed with
^C)" is more appropriate.

> +	if (use_sideband) {
> +		static const char buf[] = "0005\2";
> +		write_or_die(1, buf, sizeof(buf) - 1);
> +	}

Observing how execute_commands() and helper functions report an
error to the callers higher in the call chain, and ask them to abort
the remainder of the operation, I am not sure if write_or_die() is
appropriate.

    Side note: inside copy_to_sideband(), which runs in async, it is
    a different matter (i.e. the main process on our side is not
    what gets killed by that _or_die() part of the call), but this
    one kills the main process.

The convention around this code path seems to be to fill explanation
of error in cmd->error_string and return to the caller.  In this
case, the error_strings may not reach the pusher via report() or
report_v2() as they may have disconnected, but calling the report()
functions is not the only thing the caller will want to do after
calling us, so giving it a chance to clean up may be a better
design, e.g.

	if (write_in_full(...) < 0) {
		for (cmd = commands; cmd; cmd = cmd->next)
	        	cmd->error_string = "pusher went away";
		return;
	}

Yes, the current code will not actually use the error string in any
useful way in this particular case, since report() or report_v2()
will have nobody listening to them.  But being consistent will help
maintaining the caller, as it can later be extended to use it
locally (e.g. log the request and its outcome, check which cmd has
succeeded and failed using the NULL-ness of cmd->error_string, etc.)