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

From: Denton Liu <liu.denton@gmail.com>
Date: May 19, 2020, 21:14 UTC
Message-ID: <20200519211456.GA5091@generichostname>
In-Reply-To: <20200519184004.GA88536@coredump.intra.peff.net>

Hi Peff,

On Tue, May 19, 2020 at 02:40:04PM -0400, Jeff King wrote:

Show 9 quoted lines

> 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