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?
