Re: [PATCHv3 2/9] send-pack: Attempt to retrieve remote status even if pack-objects fails
On Mon, May 16, 2011 at 12:07:45AM -0400, Jeff King wrote:
Show 13 quoted lines
> But what if we are not using pipes, but have an actual TCP socket? In
> that case, I'm not sure what happens. We don't seem to do a half-duplex
> shutdown() anywhere. So I'm concerned that we are still open for sending
> from the remote's perspective, and we may deadlock.
>
> However, that would not necessarily be something introduced by your
> patch; you would deadlock in receive_status, but prior to that it would
> deadlock in the sideband demuxer.
>
> AFAICT, the only way to have an actual TCP connection instead of pipes
> is for the push to go over git://, which is enabled almost nowhere. But
> we should perhaps check for deadlock on failed pack-objects in that
> case, both with and without your patch.
Ugh, yeah, yet another deadlock. I can reproduce reliably with this:
[in one terminal]
mkdir daemon &&
git init --bare daemon/repo.git &&
git --git-dir=daemon/repo.git config daemon.receivepack true &&
git daemon --base-path=$PWD/daemon --export-all --verbose
[in another]
git init repo &&
cd repo &&
git remote add origin git://localhost/repo.git &&
echo content >file && git add file && git commit -a -m one &&
git push -f origin HEAD &&
echo content >>file && git commit -a -m two &&
sha1=`git rev-parse HEAD:file` &&
file=`echo $sha1 | sed 's,..,&/,'` &&
rm -fv .git/objects/$file &&
git push
and this patch fixes it:
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index e2f4e21..b9da044 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -345,6 +345,13 @@ int send_pack(struct send_pack_args *args,
ref->status = REF_STATUS_NONE;
if (args->stateless_rpc)
close(out);
+ /* in case we actually have a full-duplex socket
+ * and not two pipes; we can't use "out" because
+ * it has been closed already, but in the full-duplex
+ * case, "in" and "out" are merely dups of each other.
+ * We can't directly use "in" because it may be
+ * pointing to the sideband demuxer now */
+ shutdown(fd[0], SHUT_WR);
if (use_sideband)
finish_async(&demux);
return -1;
It does call shutdown() on a non-socket in the pipe case. That should be
a harmless noop, AFAIK.
-Peff
Jeff King <peff@peff.net>