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

[Jeff King <peff@peff.net>]

On Mon, May 18, 2020 at 11:47:17AM -0400, Denton Liu wrote:

> Changes since v1:
> 
> * Remove fallthrough in switch in favour of just extracting the common
>   call out of the switch in patch 3
> 
> * Add more detail in function comment and use `const char linelen[4]` in
>   patch 4
> 
> * Implement most of Peff's suggestions[0] in patch 5
> 
> * Only operate on stateless_connect() in patch 5
> 
> * Add tests in patch 5
> 
> * Drop "remote-curl: ensure last packet is a flush" in favour of
>   "stateless-connect: send response end packet"

Overall this looks pretty cleanly done. I left a few minor comments
throughout, but the real question is whether we prefer the "0002" packet
in the last one, or if we instead insist that the response end in a
flush.

At first glance, the "0002" seems like it's more flexible, because we're
making fewer assumptions about what's being transferred over the
stateless-connect channel. But in reality it still has to be pktlines
(because we're checking them for incomplete or invalid packets already).
So all it really buys us is that the server response doesn't have to end
with a flush packet.

So I dunno. The "0002" solution is slightly more flexible, but I'm not
sure it helps in practice. And it does eat up one of our two remaining
special packet markers.

There is another solution, which would allow arbitrary data over
stateless-connect: adding an extra level of pktline framing between the
helper and the parent process. But that's rather ugly (inner pktlines
may be split across outer ones, so you have to do a bunch of buffer
reassembly). I think that's actually how v0 http works, IIRC.
IMHO it probably isn't worth pursuing. That extra layer wrecks the
elegance to the v2 stateless-connect approach, and we really do expect
only pktlines to go over it.

So I think either of your solutions (enforcing a final flush, or the
0002 packet) is preferable. I'm on the fence between them.

-Peff