[PATCH v2] send-pack: avoid deadlock when pack-object dies early

[Johannes Sixt <j6t@kdbg.org>]

Send-pack deadlocks in two ways when pack-object dies early (for example,
because there is some repo corruption).

The first deadlock happens with the smart push protocol (--stateless-rpc).
After the initial rev-exchange, the remote is waiting for the pack data
to arrive, and the sideband demuxer at the local side continues trying to
stream data from the remote repository until it gets EOF. Meanwhile,
send-pack (in function pack_objects()) has noticed that pack-objects did
not produce output and died. Back in send_pack(), it now tries to clean
up the sideband demuxer using finish_async(). The demuxer, however, waits
for the remote end to close down, the remote waits for pack data, and
the reason that it still waits is that send-pack forgot to close the
outgoing channel. Add the missing close() in pack_objects().

The second deadlock happens in a similar constellation when the sideband
demuxer runs in a forked process (rather than in a thread). Again, the
remote end waits for pack data to arrive, the sideband demuxer waits for
the remote to shut down, and send-pack (in the regular clean-up) waits for
the demuxer to terminate. This time, the send-pack parent process closes
the writable end of the outgoing channel (in start_command() that spawned
pack-objects) so that after the death of the pack-objects process all
writable ends should have been closed and the remote repo should see EOF.
This does not happen, however, because when the sideband demuxer was forked
earlier, it also inherited a writable end; it remains open and keeps the
remote repo from seeing EOF. To break this deadlock, close the writable end
in the demuxer.

Analyzed-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 25.04.2011 18:50, schrieb Jeff King:
> In the comments for 1/2, you said this goes directly on 38a81b4e. But in
> that commit, we use #ifndef WIN32 to decide whether or not to fork for
> async code. So shouldn't this use the same test (I don't even see
> ASYNC_AS_THREAD defined anywhere else)?

Here's the fixed patch. I squashed both earlier patches into a single patch
because they are about the same topic, as you showed with your tests of
git-push via smart http.

Again, this should go on top of 38a81b4e. When it is merged to f6b60983 or
later, the '#ifndef WIN32' must be changed to '#ifdef NO_PTHREADS'.

-- Hannes

 builtin-send-pack.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 2478e18..6516288 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -97,6 +97,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 		free(buf);
 		close(po.out);
 		po.out = -1;
+		close(fd);
 	}
 
 	if (finish_command(&po))
@@ -375,6 +376,9 @@ static void print_helper_status(struct ref *ref)
 static int sideband_demux(int in, int out, void *data)
 {
 	int *fd = data;
+#ifndef WIN32
+	close(fd[1]);
+#endif
 	int ret = recv_sideband("send-pack", fd[0], out);
 	close(out);
 	return ret;
-- 
1.7.5.rc1.97.ge0653