Re: [PATCH v2] fetch-pack: try harder to read an ERR packet

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 28, 2020, 21:02 UTC
Message-ID: <xmqqh7x31fyu.fsf@gitster.c.googlers.com>
In-Reply-To: <20200428204908.GA4000@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 15 quoted lines

>> Hmm, if the connection gets severed just before the ERR packet the
>> other side has written, we will see "Broken pipe" if we write
>> "done", and no amount of "try to read to collect as much what they
>> said as possible" would help.  If you are lucky and the connection
>> is broken after the ERR reaches on this side, such an "extra effort"
>> may help, but is it really worth the effort?  It is not clear to me
>> if the extra complexity, one more API function people need to learn,
>> and the need to think which one to use every time they want to say
>> write_in_full(), are justifiable.
>
> I think the "lucky" case happens pretty routinely. The situation we're
> trying to catch here is that server does:
>
>    packet_write("ERR I don't like your request for some reason");
>    die("exiting");

OK, if we assume that the communication route is never flaky and everything writtten will go through before TCP shutdown that would happen when die() kills the process (or like test environment that most communication goes locally between two processes), sure, it may look common enough to be worth "fixing". I simply did not realize that was the shared assumption behind this patch before I went back to the original discussion that was about a racy test.

As long as an extra read on the side that just got a write error won't throw us into a deadlock, I think I am OK, but I am still not sure if the code complexity to have two write_in_full() is worth it.

If the mechanism to do this were limited to the packet IO layer, it may be more palatable, though.