Re: [RFC PATCH] http-backend: write error packet if backend command fails

[Jeff King <peff@peff.net>]

On Sat, Mar 28, 2020 at 11:13:00AM -0400, Jeff King wrote:

> On Sat, Mar 28, 2020 at 10:57:42AM -0400, Jeff King wrote:
> 
> > But since it works for v1 (which also dies!), and since you were able to
> > reproduce the problem locally, I suspect it may be an issue in
> > upload-pack itself.
> 
> Actually, I think the problem is on the client side.
> 
> If I run your test without the http-backend change, then nothing is left
> running on the server side, but on the client we have two processes:
> git-clone and the git-remote-https helper. And they're deadlocked trying
> to read from each other.
> 
> I think the issue is that git-remote-https in v2 mode calls into
> stateless_connect(), and just pumps http request/response pairs from
> git-clone to the server. But if a request generates an empty response,
> then clone has no idea that anything was received at all. It continues
> waiting, but remote-https has looped to expect another request from it.
> 
> Your patch fixes _this case_ because it causes the server to send a
> non-empty response. But the client can't rely on that. Besides that not
> being a documented server behavior, in the worst case the connection
> could be severed mid-stream. So I think remote-https needs to complain
> about an empty response. This isn't a problem in v1 because it would
> actually try to look at that empty response; in v2 it's just proxying
> bytes around.

Ugh, it's actually much worse than this. We dealt with the
empty-response case in 296b847c0d (remote-curl: don't hang when a server
dies before any output, 2016-11-18). The issue here is that we got a
_partial_ response, and clone is waiting for the rest of it.

The server said "0011shallow-info\n" before it bailed. So from the
perspective of git-clone, it's now waiting to read through a flush
packet. But remote-https has nothing left to send. The root of the issue
is that it has no way to signal to clone that it has already sent
everything the server gave it. In non-helper code, clone would see the
EOF. And in v1 https, I think there's some extra framing between
remote-https and "fetch-pack --stateless-rpc" so that EOF effectively
gets passed along. But v2's stateless_connect() strategy is
fundamentally missing this signal, and there are probably other spots in
the protocol that could cause similar deadlocks.

I wonder if remote-https's stateless_connect() could complain if there's
no flush packet in the output it writes back. That would certainly fix
this case, but I'd worry there are rpc messages that don't end in a
flush. And it would be susceptible to data cut-offs that happened to
look like a flush packet.

I think the robust solution is to introduce an extra level of pktline
framing into the remote-helper stateless-connect protocol.

-Peff