Re: [PATCH v2] receive-pack: add option to interrupt pre-receive when client exits

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

Junio C Hamano, Jan 27, 2022 at 05:36:
> I somehow feel that it is unrealistic to expect the command to be
> killed via SIGPIPE because there is no guarantee that the command
> has that many bytes to send out to to get the signal in the first
> place.  Such an expectation is simply wrong, isn't it?

Maybe I did not word that properly. Indeed, this only applies if
pre-receive has bytes to send out in the first place. This is what
I referred to with the last paragraph:

> > This does not guarantee that all client disconnections will abort
> > a push. If there is no pre-receive hook or if it does not produce
> > any output, receive-pack will not be killed via SIGPIPE and the push
> > will complete.

It would be much better not to rely on pre-receive to have bytes to send
and to expect that receive-pack will receive SIGPIPE when forwarding
them after the client has disconnected.

I thought of sending a "keepalive packet" in the socket *after* the
pre-receive hook has completed. I do not know the protocol details.
Would something like this be suitable:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8718a6dd91b4..2e0ddd1a59fe 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1990,16 +1990,28 @@ static void execute_commands(struct command *commands,
 	if (run_receive_hook(commands, "pre-receive", 0, push_options)) {
 		for (cmd = commands; cmd; cmd = cmd->next) {
 			if (!cmd->error_string)
 				cmd->error_string = "pre-receive hook declined";
 		}
 		return;
 	}
 
+	/*
+	 * Send a keepalive packet to ensure that the client has not
+	 * disconnected while pre-receive was running.
+	 */
+	{
+		static const char buf[] = "0001";
+		if (use_sideband)
+			send_sideband(1, 1, buf, sizeof(buf) - 1, use_sideband);
+		else
+			write_or_die(1, buf, sizeof(buf) - 1);
+	}
+
 	/*
 	 * Now we'll start writing out refs, which means the objects need
 	 * to be in their final positions so that other processes can see them.
 	 */
 	if (tmp_objdir_migrate(tmp_objdir) < 0) {
 		for (cmd = commands; cmd; cmd = cmd->next) {
 			if (!cmd->error_string)
 				cmd->error_string = "unable to migrate objects to permanent storage";

In that situation, if the client has exited, receive-pack should be
killed via SIGPIPE before completing the push.

> Is it safe to kill(2) from within a signal handler?

Even if it is, it is probably not a good idea. I did that to avoid
leaving a zombie after receive-pack has died. Maybe setting a flag in
the signal handler and checking the flag after the process has exited
would have been better.

> Why does this patch do anything more than a partial reversion of
> ec7dbd14 (receive-pack: allow hooks to ignore its standard input
> stream, 2014-09-12), i.e. "if the configuration says do not be
> lenient to hooks that do not consume their input, do not ignore
> sigpipe at all".

Indeed it is a partial reversion of that commit. Maybe the "keepalive
before migrating to permanent storage" solution is better.

What do you think?