On 2016-07-25 6:22 PM, Jeff King wrote:
On Mon, Jul 25, 2016 at 12:34:04PM +0200, Jan Smets wrote:
I have always assumed the post-receive hook to be executed whenever a commit
is "accepted" by the (gitolite) server. That does not seem to be true any
more.
Generally, yeah, I would expect that to be the case, too.
Since 9658846 is appears that, when a client bails out, the pre-receive hook
continues to run and the commit is written to the repository, but no
post-receive hook is executed. No signal of any kind is received in the
hook, not even a sig pipe when the post- hook is writing to stdout whilst
the client has disconnected.
I see. The problem is that cmd_receive_pack() does this:
execute_commands(commands, unpack_status, &si);
if (pack_lockfile)
unlink_or_warn(pack_lockfile);
if (report_status)
report(commands, unpack_status);
run_receive_hook(commands, "post-receive", 1);
run_update_post_hook(commands);
It reports the status to the client, and _then_ runs the post-receive
hook. But if that reporting fails (either because of an error, or if we
just get SIGPIPE because the client hung up), then we don't actually run
the hooks.
Leaving 9658846 out of it entirely, it is always going to be racy
whether we notice that the client hung up during the pre-receive step.
E.g.:
- your pre-receive might not write any output, so the muxer has
nothing to write to the other side, and we never notice that the
connection closed until we write the status out in report()
- if NO_PTHREADS is set, the sideband muxer runs in a sub-process, not
a sub-thread. And thus we don't know of a hangup except by checking
the result of finish_async(), which we never do.
- the client could hang up just _after_ we've written the pre-receive
output, but before report() is called, so there's nothing to notice
until we're in report()
So I think 9658846 just made that race a bit longer, because it means
that a write error in the sideband muxer during the pre-receive hook
means we return an error via finish_async() rather than unceremoniously
calling exit() from a sub-thread. So we have a longer period during
which we might actually finish off execute_commands() but not make it
out of report().
And the real solution is to make cmd_receive_pack() more robust, and try
harder to run the hooks even when the client hangs up or we have some
other reporting error (because getting data back to the user is only one
use of post-receive hooks; they are also used to queue jobs or do
maintenance).
But that's a bit tricky, as it requires report() to ignore SIGPIPE, and
to stop using write_or_die() or any other functions that can exit (some
of which happen at a lower level). Plus if a client does hangup, we
don't want our hook to die with SIGPIPE either, so we'd want to proxy
the data into /dev/null.
-Peff
Sounds tricky. I do think it's important, almost a 'data integrity'
issue, that IF a commit is received, THEN the post-receive hook must be
run. Too much mission-critical stuff is based on post-receive hooks.
The alternatives, as I see them --either document that the post-receive
hook cannot be fully trusted and that all such uses must change to
asynchronous polling, or otherwise just say that nobody should hit
Ctrl-C during a push (not even reflexively when their lizard-brain says
"Woops, no!") and hope that network issues don't cause the same thing--
are simply not realistic.
Stephen
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html