Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [RFC PATCH] http-backend: write error packet if backend command fails
Date: Sat, 28 Mar 2020 11:49:36 -0400
Message-ID: <20200328154936.GA1217052@coredump.intra.peff.net> (raw)
In-Reply-To: <20200328151300.GA1215566@coredump.intra.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

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-28  2:50 Denton Liu
2020-03-28 14:57 ` Jeff King
2020-03-28 15:13   ` Jeff King
2020-03-28 15:49     ` Jeff King [this message]
2020-03-29  5:34       ` Denton Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200328154936.GA1217052@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=liu.denton@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git