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

[Jeff King <peff@peff.net>]

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

> 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