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

["Robin Jarry" <robin.jarry@6wind.com>]

Junio C Hamano, Jan 28, 2022 at 18:52:
> 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.

This is not perfect but I think this is the best we can do without
adding a new capability so that the client sends a reply to the
keepalive packet.

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

OK, I will change that.

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

The main receive-pack process will be killed by SIGPIPE anyway but I can
fill the error_string fields and return for code consistency.

I'll send a v4, thanks for the review.