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

From: Jeff King <peff@peff.net>
Date: Apr 28, 2020, 20:49 UTC
Message-ID: <20200428204908.GA4000@coredump.intra.peff.net>
In-Reply-To: <xmqqzhav1kix.fsf@gitster.c.googlers.com>

On Tue, Apr 28, 2020 at 12:24:06PM -0700, Junio C Hamano wrote:

Show 22 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
> 
> > From: SZEDER Gábor <szeder.dev@gmail.com>
> >
> > When the server has hung up after sending an ERR packet to the
> > client, the client might still be writing, for example a "done"
> > line. Therefore the client might get a write error before reading
> > the ERR packet.
> >
> > When fetching, this could result in the client displaying a
> > "Broken pipe" error, instead of the more useful error sent by
> > the server in the ERR packet.
> 
> 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");

On the client side, we'll get that final packet and then see that the connection is closed. So if we get EPIPE or similar on write(), that means we're seeing the closed connection. Which means we'll _always_ have gotten the ERR packet (in this situation).

So the problem it is solving is that there isn't really flow control in the protocol. The server might be aborting and dumping out an error response while the client is still writing. If the server were to continue reading the client request before closing the connection, this wouldn't happen. And that's what an HTTP server would be doing.

But I think that's pretty tricky to do in our programs, where the protocol framing isn't so structured, and deciding when the client is done talking often involves parsing their request. E.g., imagine we die with "not our ref" due to a "want" line. We'd have to return an error up the stack to the code that is reading want/have/done lines, so that it knows to keep pumping the socket until it gets to a break point, and _then_ die().

I think by contrast, just having the client handle EPIPE more gracefully is a simpler fix.

-Peff