Re: [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects

From: Denton Liu <liu.denton@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects
Date: Tue, 19 May 2020 17:14:56 -0400	[thread overview]
Message-ID: <20200519211456.GA5091@generichostname> (raw)
In-Reply-To: <20200519184004.GA88536@coredump.intra.peff.net>

Hi Peff,

On Tue, May 19, 2020 at 02:40:04PM -0400, Jeff King wrote:
> It _seems_ to work, because that's the case your invalid-shallow test is
> covering. I'm just not sure if it works consistently, or what error we
> might produce in some cases (e.g., saying "woah, what's the weird 0002
> packet" instead of "the server response ended unexpectedly" or
> something).
> 
> I suspect any remaining issues there are cosmetic, and it might be OK to
> just discover them organically through use. But if you happen to have
> done some poking around there, it would be nice to hear it.

From what I can tell, every time a packet is read using the reader in
do_fetch_pack_v2(), we're careful about checking the packet type so we
should be safe there. Also, when piping stuff over to index-pack and
unpack-objects, it looks like the resulting call to recv_sideband()
treats any control packets as flush packets so it should handle the 0002
fine.

I could have missed checking some spots, though. But as far as I can
tell, if it can't handle the 0002 properly, it was already buggy to
begin with. I agree that we can let any remaining issues be shaken out
through use.

> Thanks for working on this. When we had the initial discussion, I was
> really worried the solution was going to be quite nasty, but I think it
> turned out to be reasonably nice.

Thanks,

Denton

> -Peff