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

From: Jeff King <peff@peff.net>
Date: May 19, 2020, 18:40 UTC
Message-ID: <20200519184004.GA88536@coredump.intra.peff.net>
In-Reply-To: <cover.1589885479.git.liu.denton@gmail.com>

On Tue, May 19, 2020 at 06:53:53AM -0400, Denton Liu wrote:

Show 13 quoted lines

> Changes since v2:
> 
> * Use if-else tower in "transport: extract common fetch_pack() call"
> 
> * Rename to `lenbuf_hex` and remove null-terminator sentence in
>   "pkt-line: extern packet_length()"
> 
> * Fix "a a" typo in "remote-curl: error on incomplete packet"
> 
> * Don't send flush packet after response end packet
> 
> * Move stateless delimiter checks closer to where message processing
>   happens in do_fetch_pack_v2()

This looks good to me, modulo the minor comment fixup from Eric. I did have one final question, though. Our discussion focused a lot on checking the 0002 packets in the success case. But we didn't discuss how fetch-pack would deal with an unexpected 0002 packet (i.e., the case that the server response is truncated, but then remote-curl tacks on its 0002).

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.

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.

-Peff