Re: [PATCH/RFC 1/2] send-pack --stateless-rpc: properly close the outgoing channel

From: Jeff King <peff@peff.net>
Date: Apr 25, 2011, 16:40 UTC
Message-ID: <20110425164038.GA1589@sigill.intra.peff.net>
In-Reply-To: <4DB48B2C.2090904@kdbg.org>

On Sun, Apr 24, 2011 at 10:42:20PM +0200, Johannes Sixt wrote:

Show 11 quoted lines

> In the non-stateless-rpc case, the writable end of the channel to the
> remote repo is closed by the start_command() call that runs the
> pack-objects process (after pack-objects inherited a copy). But in the
> --stateless-rpc case, where send-pack takes care of writing data to the
> channel, this was missed.
>
> [...]
>
> Warning: This patch is untested. Furthermore, it does not even fix a resource
> leak because the fd that is now closed in pack_objects() would be closed
> later in cmd_send_pack.

Note that we can also call send_pack directly from git-push via the transport.c interface. I didn't check whether one can actually trigger stateless-rpc this way, though; it looks like git-remote-http ends up exec'ing a separate send-pack.

Show 5 quoted lines

> However, without closing the fd earlier like this, a --stateless-rpc
> invocation could theoretically dead-lock just like a regular
> invocation in a NO_PTHREADS build. But I also don't know how to
> test-drive send-pack --stateless-rpc to construct such a case. Any
> hints how to do that would be appreciated.

I was able to get a hang using v1.7.5 compiled with pthreads. You need to have a server accepting smart-http push (if you use github, you can create an empty test repo there, which is sufficient).

And then do a modified version of the test I posted earlier:

  UPSTREAM=https://peff@github.com/peff/test.git

  git init child &&
  cd child &&
  git remote add origin $UPSTREAM &&
  echo content >file &&
  git add file &&
  git commit -m one &&
  echo content >>file &&
  git add file &&
  git commit -m two &&
  sha1=`git rev-parse HEAD:file` &&
  file=`echo $sha1 | sed 's,..,&/,'` &&
  rm -fv .git/objects/$file

where obviously you need to tweak $UPSTREAM to point to the repo you created. That sets up the broken repo state. You can then try "git push" in the repo with various versions to check their behavior.

With stock v1.7.5, this will hang after pack-objects reports the fatal error. With your patch, it exits immediately, though the output looks like this:

  $ git push
  Password:
  Counting objects: 5, done.
  error: unable to find ea0faeb6073ff6cb085727c3647be457051e6ed7
  fatal: unable to read ea0faeb6073ff6cb085727c3647be457051e6ed7
  fatal: The remote end hung up unexpectedly
  fatal: The remote end hung up unexpectedly
  fatal: write error: Bad file descriptor

which could perhaps be a little nicer, but is probably not a big deal (I didn't dig very deep, but presumably we should exit a little more immediately after seeing pack-objects fail).

-Peff