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:

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