| Message ID | 20211106220358.144886-1-robin@jarry.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] receive-pack: ignore SIGPIPE while reporting status to client | expand |
Robin Jarry <robin@jarry.cc> writes: > When a remote client exits while the pre-receive hook is running, > receive-pack is not killed by SIGPIPE because the signal is ignored. > This is a side effect of commit ec7dbd145bd8 (receive-pack: allow hooks > to ignore its standard input stream, 2014-09-12). > > The pre-receive hook itself is not interrupted and does not receive any > error since its stdout is a pipe which is read in an async thread and > output back to the client socket in a side band channel. > > After the pre-receive has exited the SIGPIPE default handler is restored > and if the hook did not report any error, objects are migrated from > temporary to permanent storage. All of the above talks about the pre-receive hook, but it is unclear how that is relevant to this change. The first paragraph says "... is not killed", and if that was a bad thing (in other words, it should be killed but is not, and that is a bug worth fixing), and if this patch changes the behaviour, then that paragraph is worth saying. Similarly for the other two. > Before running the post-receive hook, status info is reported back to > the client. Since the client has died, receive-pack is killed by SIGPIPE > and post-receive is never executed. In other words, regardless of what happens (or does not happen) to the pre-receive hook, which may not even exist, if "git push" dies before the post-receive hook runs, this paragraph applies, no? What I am getting at is that this can (and should) be the first paragraph of the description without losing clarity. > Ignore SIGPIPE before reporting status to the client to increase the > chances of post-receive running if pre-receive was successful. This does > not guarantee 100% consistency but it should resist early disconnection > by the client. OK. > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 49b846d96052..5fe7992028d4 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -2564,12 +2564,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > use_keepalive = KEEPALIVE_ALWAYS; > execute_commands(commands, unpack_status, &si, > &push_options); > + sigchain_push(SIGPIPE, SIG_IGN); > if (pack_lockfile) > unlink_or_warn(pack_lockfile); Shouldn't we start ignoring SIGPIPE here, not before we try to unlink the lockfile? > if (report_status_v2) > report_v2(commands, unpack_status); > else if (report_status) > report(commands, unpack_status); > + sigchain_pop(SIGPIPE); In other words, push/pop pair should surround the part that reports the status, as the proposed commit log message said. > run_receive_hook(commands, "post-receive", 1, > &push_options); > run_update_post_hook(commands); Other than these, looks good to me. Thanks.
Hi Junio, Junio C Hamano, Nov 09, 2021 at 22:10: > All of the above talks about the pre-receive hook, but it is unclear > how that is relevant to this change. The first paragraph says > "... is not killed", and if that was a bad thing (in other words, it > should be killed but is not, and that is a bug worth fixing), and if > this patch changes the behaviour, then that paragraph is worth > saying. Similarly for the other two. > > > Before running the post-receive hook, status info is reported back to > > the client. Since the client has died, receive-pack is killed by SIGPIPE > > and post-receive is never executed. > > In other words, regardless of what happens (or does not happen) to > the pre-receive hook, which may not even exist, if "git push" dies > before the post-receive hook runs, this paragraph applies, no? > > What I am getting at is that this can (and should) be the first > paragraph of the description without losing clarity. You're right. I wanted to give context to better explain why receive-pack is not killed while running the pre-receive hook but afterwards. This should go into another commit which fixes that issue. I will reword accordingly. > > execute_commands(commands, unpack_status, &si, > > &push_options); > > + sigchain_push(SIGPIPE, SIG_IGN); > > if (pack_lockfile) > > unlink_or_warn(pack_lockfile); > > Shouldn't we start ignoring SIGPIPE here, not before we try to > unlink the lockfile? I initially wanted to avoid getting SIGPIPE'd while printing a warning if the lockfile cannot be unlinked. Maybe this means the repository integrity is compromised and we are well beyond ensuring post-receive is executed or not. I do not know git internals well enough to be sure. What do you think?
"Robin Jarry" <robin@jarry.cc> writes: >> > + sigchain_push(SIGPIPE, SIG_IGN); >> > if (pack_lockfile) >> > unlink_or_warn(pack_lockfile); >> >> Shouldn't we start ignoring SIGPIPE here, not before we try to >> unlink the lockfile? > > I initially wanted to avoid getting SIGPIPE'd while printing a warning > if the lockfile cannot be unlinked. Maybe this means the repository > integrity is compromised and we are well beyond ensuring post-receive is > executed or not. I do not know git internals well enough to be sure. > > What do you think? I think that push/pop pair should surround the part that reports the status, as the proposed commit log message said. Thanks.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 49b846d96052..5fe7992028d4 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2564,12 +2564,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) use_keepalive = KEEPALIVE_ALWAYS; execute_commands(commands, unpack_status, &si, &push_options); + sigchain_push(SIGPIPE, SIG_IGN); if (pack_lockfile) unlink_or_warn(pack_lockfile); if (report_status_v2) report_v2(commands, unpack_status); else if (report_status) report(commands, unpack_status); + sigchain_pop(SIGPIPE); run_receive_hook(commands, "post-receive", 1, &push_options); run_update_post_hook(commands);
When a remote client exits while the pre-receive hook is running, receive-pack is not killed by SIGPIPE because the signal is ignored. This is a side effect of commit ec7dbd145bd8 (receive-pack: allow hooks to ignore its standard input stream, 2014-09-12). The pre-receive hook itself is not interrupted and does not receive any error since its stdout is a pipe which is read in an async thread and output back to the client socket in a side band channel. After the pre-receive has exited the SIGPIPE default handler is restored and if the hook did not report any error, objects are migrated from temporary to permanent storage. Before running the post-receive hook, status info is reported back to the client. Since the client has died, receive-pack is killed by SIGPIPE and post-receive is never executed. The post-receive hook is often used to send email notifications (see contrib/hooks/post-receive-email), update bug trackers, start automatic builds, etc. Not executing it after an interrupted yet "successful" push can lead to inconsistencies. Ignore SIGPIPE before reporting status to the client to increase the chances of post-receive running if pre-receive was successful. This does not guarantee 100% consistency but it should resist early disconnection by the client. Signed-off-by: Robin Jarry <robin@jarry.cc> --- Ideally, pre-receive should not be allowed to succeed if the client has disconnected before objects have been migrated from temporary to permanent storage. But that is another topic, and I think it would complement this patch. builtin/receive-pack.c | 2 ++ 1 file changed, 2 insertions(+)