Re: t5541: Bad file descriptor

[Jeff King <peff@peff.net>]

[argh, resend, I meant to cc Johannes Sixt]

On Thu, May 05, 2011 at 01:46:11AM -0400, Jeff King wrote:

> On Wed, May 04, 2011 at 10:35:13PM -0700, Junio C Hamano wrote:
> 
> > Brian Gernhardt <brian@gernhardtsoftware.com> writes:
> > 
> > > I haven't had a lot of time to track down it down until today, but I've
> > > been getting failures in t5541-http-push-.sh.  Several tests fail with
> > > the error "fatal: write error: Bad file descriptor".
> > 
> > A wild guess.
> > 
> > Does it help if you cherry-picked 1e41827 (http: clear POSTFIELDS when
> > initializing a slot, 2011-04-26) on top of the faulty commit?
> 
> I think that 09c9957 (send-pack: avoid deadlock when pack-object dies
> early, 2011-04-25) is totally broken.
> 
> Looking back at my tests, I only tested the case where pack-objects
> fails. And it seems we totally broke the case where the push is supposed
> to succeed.
> 
> Still investigating...

OK, embarrassing. 09c9957 completely breaks smart http pushing. My
testing of Johannes' patch was completely focused on the error case, and
I didn't have a single test for the non-error case. And on top of that,
we _have_ nice tests in the test suite to catch this, but obviously
neither I, nor Johannes, nor Junio were running them (because they need
apache installed and GIT_TEST_HTTPD set).

Ugh.

This patch on top of 09c9957 should fix it.

-- >8 --
Subject: [PATCH] send-pack: unbreak push over stateless rpc

Commit 09c9957 (send-pack: avoid deadlock when pack-object
dies early, 2011-04-25) attempted to fix a hang in the
stateless rpc case by closing a file descriptor early, but
we still need that descriptor.

Basically the deadlock can happen when pack-objects fails,
and the descriptor to upstream is left open. We never send
the pack, so the upstream is left waiting for us to say
something, and we are left waiting for upstream to close the
connection.

In the non-rpc case, our descriptor points straight to the
upstream. We hand it off to run-command, which takes
ownership and closes the descriptor after pack-objects
finishes (whether it succeeds or not).

Commit 09c9957 tried to emulate that in the rpc case. That
isn't right, though. We actually have a descriptor going
back to the remote-helper, and we need to keep using it
after pack-objects is finished. Closing it early completely
breaks pushing via smart-http.

We still need to do something on error to signal the
remote-helper that we won't be sending any pack data
(otherwise we get the deadlock).  In an ideal world, we
would send a special packet back that says "Sorry, there was
an error". But the remote-helper doesn't understand any such
packet, so the best we can do is close the descriptor and
let it report that we hung up unexpectedly.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-send-pack.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 6516288..3e70795 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -97,7 +97,6 @@ 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))
@@ -519,6 +518,8 @@ int send_pack(struct send_pack_args *args,
 		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
 			for (ref = remote_refs; ref; ref = ref->next)
 				ref->status = REF_STATUS_NONE;
+			if (args->stateless_rpc)
+				close(out);
 			if (use_sideband)
 				finish_async(&demux);
 			return -1;
-- 
1.7.5.406.gfbb2