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

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

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.

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