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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 28, 2022, 17:52 UTC
Message-ID: <xmqq4k5nychf.fsf@gitster.g>
In-Reply-To: <20220127215553.1386024-1-robin.jarry@6wind.com>

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

Show 11 quoted lines

> 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.

Show 21 quoted lines

> 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.)