From 09c9957cf77106ca6b49127d5df20080922c81a4 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Mon, 25 Apr 2011 23:04:10 +0200 Subject: send-pack: avoid deadlock when pack-object dies early 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 Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano 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; -- cgit v0.10.2-6-g49f6 From e07fd15b1295393ddf954b9fdd0c3a961e5509c5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 May 2011 02:18:45 -0400 Subject: send-pack: unbreak push over stateless rpc Commit 09c9957 (send-pack: avoid deadlock when pack-object dies early, 2011-04-25) attempted to fix a hang in the stateless rpc case by closing a file descriptor early, but we still need that descriptor. Basically the deadlock can happen when pack-objects fails, and the descriptor to upstream is left open. We never send the pack, so the upstream is left waiting for us to say something, and we are left waiting for upstream to close the connection. In the non-rpc case, our descriptor points straight to the upstream. We hand it off to run-command, which takes ownership and closes the descriptor after pack-objects finishes (whether it succeeds or not). Commit 09c9957 tried to emulate that in the rpc case. That isn't right, though. We actually have a descriptor going back to the remote-helper, and we need to keep using it after pack-objects is finished. Closing it early completely breaks pushing via smart-http. We still need to do something on error to signal the remote-helper that we won't be sending any pack data (otherwise we get the deadlock). In an ideal world, we would send a special packet back that says "Sorry, there was an error". But the remote-helper doesn't understand any such packet, so the best we can do is close the descriptor and let it report that we hung up unexpectedly. Signed-off-by: Jeff King Acked-by: Johannes Sixt Signed-off-by: Junio C Hamano diff --git a/builtin-send-pack.c b/builtin-send-pack.c index 6516288..3e70795 100644 --- a/builtin-send-pack.c +++ b/builtin-send-pack.c @@ -97,7 +97,6 @@ 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)) @@ -519,6 +518,8 @@ int send_pack(struct send_pack_args *args, if (pack_objects(out, remote_refs, extra_have, args) < 0) { for (ref = remote_refs; ref; ref = ref->next) ref->status = REF_STATUS_NONE; + if (args->stateless_rpc) + close(out); if (use_sideband) finish_async(&demux); return -1; -- cgit v0.10.2-6-g49f6