list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Daniel Duvall <>
Cc: Git Users <>
Subject: Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves
Date: Fri, 30 Oct 2020 05:09:02 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Fri, Oct 30, 2020 at 12:47:29AM -0700, Daniel Duvall wrote:

> >   - the client doesn't care; by definition it has hung up at this point
> >     and will keep going with its next request
> That's true in the case where the server doesn't surface the non-zero
> exit code from git-upload-pack—which results in git-http-backend
> existing non-zero as well. An apache setup I used in testing doesn't
> seem to care about the failure—it responds 200 and so the client is
> happy—but in our (dayjob) case, we're running a Phabricator instance
> which handles a non-zero exit from git-http-backend by responding 500,
> and the client dies.
> $ git fetch --depth 1
> error: RPC failed; HTTP 500 curl 22 The requested URL returned error: 500
> fatal: the remote end hung up unexpectedly

Thanks, that's a really helpful data point. The flaw in my reasoning is
that the client connection is bidirectional: just because the client
hung up does not mean they are not still listening to the remainder of
what the server has to say.

It sounds like Phabricator should probably be ignoring the exit code
from http-backend. Or at least doing so when the CGI managed to produce
an HTTP status code, and assuming that Git's response was either
well-formed, or that the client will realize it was broken. (Or possibly
http-backend should be less eager to pass along a non-zero exit code,
but it amounts to the same thing).

But regardless, we should make sure that upload-pack is doing the right
thing at least for this case.

> > If we're _not_ going to be strict, then I actually wonder if we ought to
> > simply teach get_common_commits() that seeing an EOF is OK in stateless
> > mode, wherever it comes. It can't possibly impact the correctness of the
> > protocol conversation (since we're stateless and the client is gone),
> > but maybe it's useful if you're trying to count how often clients really
> > do hang up.
> I originally took that approach, but gently handling an EOF in the
> get_common_commits loop resulted in a NAK being sent back because of:
>                 if (packet_reader_read(reader) != PACKET_READ_NORMAL) {
>                         [...]
>                         if (data-> == 0 || data->multi_ack)
>                                 packet_write_fmt(1, "NAK\n");
>                         [...]
>                         if (data->stateless_rpc)
>                                 exit(0);
>                         [...]
>                 }
> which the client died on with an "expected shallow list" message. I
> didn't see a straightforward way of modifying the conditions _inside_
> the loop while ensuring I wasn't changing any expected behavior upon
> EOF.

I was thinking something more like:

diff --git a/upload-pack.c b/upload-pack.c
index 2b128e4ad8..f6d3ef3e13 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -520,10 +520,18 @@ static int get_common_commits(struct upload_pack_data *data,
 	for (;;) {
 		const char *arg;
+		enum packet_read_status status;
-		if (packet_reader_read(reader) != PACKET_READ_NORMAL) {
+		status = packet_reader_read(reader);
+		if (status == PACKET_READ_EOF) {
+			if (data->stateless_rpc)
+				exit(0);
+			die("the remote end hung up unexpectedly");
+		}
+		if (status != PACKET_READ_NORMAL) {
 			if (data->multi_ack == MULTI_ACK_DETAILED
 			    && got_common
 			    && !got_other

But after reading the rest of your response, I think it probably does
make sense to be more strict here, and only handle the unexpected EOF
before entering get_common_commits(). I don't _think_ it should matter
to the client, because this whole "status != PACKET_READ_NORMAL" path
would never be entered with the current code (we'd die() when we see the
EOF inside packet_reader_read()).

> > This function only handles the v0 protocol. For v2, we end up in
> > upload_pack_v2(). But my reading of the client side do_fetch_pack_v2()
> > is that it _doesn't_ send this extra request. And a simple test seems to
> > confirm it. Which gives me further pause as to whether the extra request
> > is necessary for v0.
> When I do a trace using v2, I see two roundtrip requests as well. I
> haven't tested the exit status of git-upload-pack in that case
> however. It's getting late for me but I'll investigate tomorrow.

There's an extra round-trip in v2: we probe for v2 and get the
capabilities in the initial request, then we get the ref advertisement,
then we start the object negotiation. In v0 the ref advertisement is
lumped into the initial response.

Here's what I see with:

  $ git init
  $ GIT_TRACE_CURL=$PWD/trace.out \
      git -c protocol.version=2 fetch --depth 1 \ master
  $ perl -lne '/(Send data:|Info: upload completely sent) .*/ and print $&' trace.out
  Send data: 0014command=ls-refs.0024agent=git/
  Send data: 0009peel.000csymrefs.0016ref-prefix master.001bref-prefix re
  Send data: fs/master.0020ref-prefix refs/tags/master.0021ref-prefix ref
  Send data: s/heads/master.0023ref-prefix refs/remotes/master.0028ref-pr
  Send data: efix refs/remotes/master/HEAD.001aref-prefix refs/tags/.0000
  Info: upload completely sent off: 300 out of 300 bytes
  Send data: 0011command=fetch0024agent=git/
  Send data: dthin-pack000dofs-delta000cdeepen 10032want ad27df6a5cff694a
  Send data: dd500ab8c7f97234feb4a91f.0009done.0000
  Info: upload completely sent off: 158 out of 158 bytes

So the first one POST is getting the ref advertisement, and the second
one is the full negotiation request, including the "done".

I'm still uncertain whether it could all be done in one request for v0.
But one possible solution is: let's not care. If v2 does it correctly,
that's the future anyway (or present; it's now the default in v2.29).
And the change you're proposing in upload-pack would be desirable anyway
to help deal with older clients.

If that's the route we go, we should make sure the commit message
explains it.


  reply	other threads:[~2020-10-30  9:09 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-30  1:40 Daniel Duvall
2020-10-30  2:59 ` Eric Sunshine
2020-10-30  3:31   ` Daniel Duvall
2020-10-30  3:43 ` Daniel Duvall
2020-10-30 18:42   ` Junio C Hamano
2020-10-30 22:38     ` Daniel Duvall
2020-10-30  4:40 ` Jeff King
2020-10-30  7:47   ` Daniel Duvall
2020-10-30  9:09     ` Jeff King [this message]
2020-11-03 21:10       ` Junio C Hamano
2020-11-04 13:33         ` Jeff King
2020-11-04 14:06           ` Daniel Duvall
2020-11-04 18:25             ` Junio C Hamano
2020-10-30 22:35 ` [PATCH v2] " Daniel Duvall
2020-10-30 23:45   ` Junio C Hamano
2020-10-31  2:42     ` Daniel Duvall
2020-10-31  4:17       ` Junio C Hamano
2020-10-31  2:39 ` [PATCH v3] " Daniel Duvall

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:

  List information:

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

  git send-email \ \ \ \ \
    --subject='Re: [PATCH] upload-pack: allow stateless client EOF just prior to haves' \

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

Code repositories for project(s) associated with this inbox:

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).