Re: [PATCHv3 2/9] send-pack: Attempt to retrieve remote status even if pack-objects fails

From: Jeff King <peff@peff.net>
Date: May 16, 2011, 04:07 UTC
Message-ID: <20110516040745.GA23583@sigill.intra.peff.net>
In-Reply-To: <1305495440-30836-3-git-send-email-johan@herland.net>

On Sun, May 15, 2011 at 11:37:13PM +0200, Johan Herland wrote:

Show 5 quoted lines

> I first wrote this patch on a base where e07fd15 (Peff's "send-pack:
> unbreak push over stateless rpc") was not present, and then resolved
> a conflict when rebasing this patch onto current master. I hope Peff
> or Johannes (Sixt) can verify that my patch does not reintroduce the
> deadlock they fixed.

I don't think it reintroduces the deadlock we fixed, but I am worried that it produces a new, similar one. That is, imagine pack-objects fails horribly, maybe or maybe not producing any output. We close its pipe outgoing pipe at the end of run_command, and per Johannes' 09c9957, we are sure that the sideband demuxer does not hold a pipe end open, either.

So the remote side sees us close our end of the pipe, knows there is no more pack data, and then closes their end. So our receive_status should either get some error message, or EOF, either of which is fine. So no deadlock there. Essentially, we have done a half-duplex shutdown of the connection to the remote, and that is enough for everybody to keep going.

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.

-Peff