Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error

Jeff King [off-list ref] writes:

I think I am leaning a bit towards (2a). It's simple, and it's not like
this is library code with a million unknown callers; fixing it simply
and cleanly with a nice commit message is probably sufficient.

This really sounds like a plan.  Even if we _might_ later want to go to
2b. or some other solution, we will know what pattern to grep for.

Thanks.

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.

Signed-off-by: Johannes Sixt <redacted>
---
Am 14.04.2011 23:21, schrieb Junio C Hamano:

Jeff King [off-list ref] writes:

quoted

I think I am leaning a bit towards (2a). It's simple, and it's not like
this is library code with a million unknown callers; fixing it simply
and cleanly with a nice commit message is probably sufficient.

This really sounds like a plan.  Even if we _might_ later want to go to
2b. or some other solution, we will know what pattern to grep for.

Here's a 2-patch series that implements this plan. The patches go on top of
38a81b4e (receive-pack: Wrap status reports inside side-band-64k) just like
Jeff's series (jk/maint-push-async-hang).

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. 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.

-- Hannes

 builtin-send-pack.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 2478e18..089058b 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))

-- 
1.7.5.rc1.97.ge0653

When pack-objects dies prematurely (for example, because there is some
repo corruption), we are careful to clean up the sideband demuxer (if it
is being used) with finish_async. For an async implementation which forks
(i.e., not Windows), that means we will waitpid() for the async process.

Meanwhile, the async sideband demuxer will continue trying to stream data
from the remote repo until it gets EOF. Depending on what data
pack-objects actually sent, the remote repo may not actually send
anything, in particular, after the initial rev-exchange it is waiting for
the pack data to arrive.

The send-pack parent process closes the writable end of the channel 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. This leads to a deadlock cycle in which send-pack waits
on the demuxer, the demuxer waits on the remote receive-pack, and the
remote receive-pack waits on send-pack to send the pack data.

To break this, we close the writable end in the demuxer, but only when
it runs in a forked async process.

Analyzed-by: Jeff King [off-list ref]
Signed-off-by: Johannes Sixt <redacted>
---
This is the replacement of the series that Jeff proposed earlier. It
should also be merged into f6b60983 (Enable threaded async procedures
whenever pthreads is available). To keep bisectability, the merge commit
should replace '#ifndef ASYNC_AS_THREAD' by '#ifdef NO_PTHREADS'.

-- Hannes

 builtin-send-pack.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 089058b..b371c79 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c

@@ -376,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 ASYNC_AS_THREAD
+	close(fd[1]);
+#endif
 	int ret = recv_sideband("send-pack", fd[0], out);
 	close(out);
 	return ret;

-- 
1.7.5.rc1.97.ge0653

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

On Sun, Apr 24, 2011 at 10:49:17PM +0200, Johannes Sixt wrote:

In the comments for 1/2, you said this goes directly on 38a81b4e. But in
that commit, we use #ifndef WIN32 to decide whether or not to fork for
async code. So shouldn't this use the same test (I don't even see
ASYNC_AS_THREAD defined anywhere else)?

And of course in more modern versions, it should be NO_PTHREADS, as you
noted.

-Peff

Am 25.04.2011 18:50, schrieb Jeff King:

On Sun, Apr 24, 2011 at 10:49:17PM +0200, Johannes Sixt wrote:

quoted

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 089058b..b371c79 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c

@@ -376,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 ASYNC_AS_THREAD
+	close(fd[1]);
+#endif

In the comments for 1/2, you said this goes directly on 38a81b4e. But in
that commit, we use #ifndef WIN32 to decide whether or not to fork for
async code. So shouldn't this use the same test (I don't even see
ASYNC_AS_THREAD defined anywhere else)?

Oops, you are right. I was looking at f6b60983, the one that the
two-patch series should be merged to; there, we remove ASYNC_AS_THREAD
and replace it by NO_PTHREADS. Therefore, I assumed that we use the
former symbol to decide whether to use threaded async procedures.
Obviously, the symbol was introduced only later. Will resend.

-- Hannes

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 [off-list ref]
Signed-off-by: Johannes Sixt <redacted>
---
Am 25.04.2011 18:50, schrieb Jeff King:

In the comments for 1/2, you said this goes directly on 38a81b4e. But in
that commit, we use #ifndef WIN32 to decide whether or not to fork for
async code. So shouldn't this use the same test (I don't even see
ASYNC_AS_THREAD defined anywhere else)?

Here's the fixed patch. I squashed both earlier patches into a single patch
because they are about the same topic, as you showed with your tests of
git-push via smart http.

Again, this should go on top of 38a81b4e. When it is merged to f6b60983 or
later, the '#ifndef WIN32' must be changed to '#ifdef NO_PTHREADS'.

-- Hannes

 builtin-send-pack.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

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;

-- 
1.7.5.rc1.97.ge0653

On Mon, Apr 25, 2011 at 11:04:10PM +0200, Johannes Sixt wrote:

quoted

In the comments for 1/2, you said this goes directly on 38a81b4e. But in
that commit, we use #ifndef WIN32 to decide whether or not to fork for
async code. So shouldn't this use the same test (I don't even see
ASYNC_AS_THREAD defined anywhere else)?

Here's the fixed patch. I squashed both earlier patches into a single patch
because they are about the same topic, as you showed with your tests of
git-push via smart http.

Thanks, this one looks good to me, and I think the squash is sensible.

Again, this should go on top of 38a81b4e. When it is merged to f6b60983 or
later, the '#ifndef WIN32' must be changed to '#ifdef NO_PTHREADS'.

I see Junio has an evil merge in pu that takes care of this.

I wonder if it might have been nicer to "cherry-pick -n" the old fix on
top of master, fixing up the tree to use NO_PTHREADS, and then resolving
the ensuing merge conflict in favor of the cherry-picked version. . I
guess it is just a matter of style (the fact that a few of our
archaeology tools do not find content in evil merges very well is a
downside, but that just means we should fix the tools :) ).

-Peff