Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Denton Liu <liu.denton@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Force Charlie <charlieio@outlook.com>
Subject: Re: [PATCH v2 7/7] stateless-connect: send response end packet
Date: Mon, 18 May 2020 13:12:49 -0400
Message-ID: <20200518171249.GA2462058@generichostname> (raw)
In-Reply-To: <20200518164308.GC42240@coredump.intra.peff.net>

Hi Peff,

On Mon, May 18, 2020 at 12:43:08PM -0400, Jeff King wrote:
> On Mon, May 18, 2020 at 11:47:24AM -0400, Denton Liu wrote:

[...]

> > @@ -446,6 +447,13 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
> >  	if (reader->status != PACKET_READ_FLUSH)
> >  		die(_("expected flush after ref listing"));
> >  
> > +	if (stateless_rpc) {
> > +		if (packet_reader_read(reader) != PACKET_READ_RESPONSE_END)
> > +			die(_("expected response end packet after ref listing"));
> > +		if (packet_reader_read(reader) != PACKET_READ_FLUSH)
> > +			die(_("expected flush packet after response end"));
> > +	}
> 
> Having two packets here surprised me. We'd have already received the
> actual flush from the server (as you can see in the context above).
> Wouldn't a single RESPONSE_END be enough to signal the reader?

Yes, having a single RESPONSE_END is enough. I sent a flush packet
because it seemed appropriate when I was writing the patch to end all
messages with a flush but I suppose that's just cargo culting. I'll
remove it in the next iteration.

> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index f73a2ce6cb..bcbbb7e2fb 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -1468,6 +1468,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> >  	struct fetch_negotiator negotiator_alloc;
> >  	struct fetch_negotiator *negotiator;
> >  	int seen_ack = 0;
> > +	int check_http_delimiter;
> 
> This flag was more complicated than I expected, and I'm not sure how we
> can easily be certain that all necessary paths are covered.

Essentially, any time we're in a path where state is set to
FETCH_SEND_REQUEST or FETCH_DONE, we know that we've finished processing
the current request and we can check for response end packets. Of course,
the one exception to this is the first iteration of the loop when we're
transitioning from FETCH_CHECK_LOCAL to FETCH_SEND_REQUEST where we
can't check for response end packets since nothing has been requested
yet.

> E.g., in FETCH_PROCESS_ACKS, we'll always be reading a response via
> process_acks(). Why do COMMON_FOUND and NO_COMMON_FOUND check for
> end-of-response, but READY doesn't? I think the answer is that we'd
> continue to read the same response via FETCH_GET_PACK in this instance.
> 
> I just wonder if there is a better place to put this logic that would be
> more certain to catch every place we'd expect to read to the end of a
> response. But I suppose not. We could push it down into process_acks(),
> but it would have the same READY logic that's here. I'll admit part of
> my complaint is that the existing do_fetch_pack_v2 state-machine switch
> is kind of hard to follow, but that's not your fault.

I debated between the current implementation and doing something like

	int first_iteration = 1;

	...

	while (state != FETCH_DONE) {
		switch (...) {
			...
		}

		if (args->stateless_rpc && !first_iteration && (state == FETCH_SEND_REQUEST || state == FETCH_DONE)) {
			if (packet_reader_read(&reader) != PACKET_READ_RESPONSE_END)
				die(_("git fetch-pack: expected response end packet"));
			if (packet_reader_read(&reader) != PACKET_READ_FLUSH)
				die(_("git fetch-pack: expected flush packet"));
		}
		first_iteration = 0;
	}

I think that this catches _all_ the cases without fiddling with any of
the state machine logic.

[...]

> Also, it probably should be check_stateless_delimiter() or something.
> There could be other helpers that support stateless-connect besides
> http.

Yeah... I forgot to change the check_http_delimiter name when I was
doing cleanup. Whoops.

> Speaking of which: this is a change to the remote-helper protocol, since
> we're now expecting stateless-connect helpers to produce these delim
> packets (and failing if they don't). I kind of doubt that anybody but
> remote-curl has implemented v2 stateless-connect, but should we be
> marking this with an extra capability to be on the safe side?

I think that we're probably safe from breaking anything external.
According to the gitremote-helpers documentation, 

	'stateless-connect'::
		Experimental; for internal use only.

so I think that gives us a bit of leeway in terms of the changes we can
make to the stateless-connect protocol. They've been warned ;)

Thanks,

Denton

  reply index

Thread overview: 62+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 18:04 [PATCH 0/6] remote-curl: partial fix for a deadlock with stateless rpc Denton Liu
2020-05-13 18:04 ` [PATCH 1/6] remote-curl: fix typo Denton Liu
2020-05-13 18:04 ` [PATCH 2/6] remote-curl: remove label indentation Denton Liu
2020-05-13 18:04 ` [PATCH 3/6] transport: combine common cases with a fallthrough Denton Liu
2020-05-13 23:14   ` Eric Sunshine
2020-05-18  9:18     ` Denton Liu
2020-05-18 17:43       ` Eric Sunshine
2020-05-13 18:04 ` [PATCH 4/6] pkt-line: extern packet_length() Denton Liu
2020-05-13 23:23   ` Eric Sunshine
2020-05-15 20:56   ` Jeff King
2020-05-15 20:57     ` Jeff King
2020-05-13 18:04 ` [PATCH 5/6] remote-curl: error on incomplete packet Denton Liu
2020-05-15 21:38   ` Jeff King
2020-05-18  9:08     ` Denton Liu
2020-05-18 15:49       ` Jeff King
2020-05-13 18:04 ` [PATCH 6/6] remote-curl: ensure last packet is a flush Denton Liu
2020-05-15 21:02   ` Denton Liu
2020-05-15 21:41     ` Jeff King
2020-05-18 16:34       ` Junio C Hamano
2020-05-18 16:52         ` Jeff King
2020-05-18 21:00           ` Jeff King
2020-05-18 15:47 ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Denton Liu
2020-05-18 15:47   ` [PATCH v2 1/7] remote-curl: fix typo Denton Liu
2020-05-18 15:47   ` [PATCH v2 2/7] remote-curl: remove label indentation Denton Liu
2020-05-18 18:37     ` Junio C Hamano
2020-05-18 15:47   ` [PATCH v2 3/7] transport: extract common fetch_pack() call Denton Liu
2020-05-18 18:40     ` Junio C Hamano
2020-05-18 15:47   ` [PATCH v2 4/7] pkt-line: extern packet_length() Denton Liu
2020-05-18 16:04     ` Jeff King
2020-05-18 17:50       ` Eric Sunshine
2020-05-18 20:08         ` Jeff King
2020-05-18 18:44       ` Junio C Hamano
2020-05-18 15:47   ` [PATCH v2 5/7] remote-curl: error on incomplete packet Denton Liu
2020-05-18 16:22     ` Jeff King
2020-05-18 16:51       ` Denton Liu
2020-05-18 15:47   ` [PATCH v2 6/7] pkt-line: PACKET_READ_RESPONSE_END Denton Liu
2020-05-18 15:47   ` [PATCH v2 7/7] stateless-connect: send response end packet Denton Liu
2020-05-18 16:43     ` Jeff King
2020-05-18 17:12       ` Denton Liu [this message]
2020-05-18 17:26         ` Jeff King
2020-05-18 16:50   ` [PATCH v2 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
2020-05-18 17:36     ` Denton Liu
2020-05-18 20:58       ` Jeff King
2020-05-18 22:52         ` Junio C Hamano
2020-05-19  2:38           ` Jeff King
2020-05-18 19:36     ` Junio C Hamano
2020-05-19 10:53   ` [PATCH v3 " Denton Liu
2020-05-19 10:53     ` [PATCH v3 1/7] remote-curl: fix typo Denton Liu
2020-05-19 10:53     ` [PATCH v3 2/7] remote-curl: remove label indentation Denton Liu
2020-05-19 10:53     ` [PATCH v3 3/7] transport: extract common fetch_pack() call Denton Liu
2020-05-19 10:53     ` [PATCH v3 4/7] pkt-line: extern packet_length() Denton Liu
2020-05-19 16:23       ` Eric Sunshine
2020-05-19 10:53     ` [PATCH v3 5/7] remote-curl: error on incomplete packet Denton Liu
2020-05-19 10:53     ` [PATCH v3 6/7] pkt-line: define PACKET_READ_RESPONSE_END Denton Liu
2020-05-19 10:54     ` [PATCH v3 7/7] stateless-connect: send response end packet Denton Liu
2020-05-19 18:40     ` [PATCH v3 0/7] remote-curl: fix deadlocks when remote server disconnects Jeff King
2020-05-19 21:14       ` Denton Liu
2020-05-19 20:51     ` [PATCH v3 8/7] fixup! pkt-line: extern packet_length() Denton Liu
2020-05-22 13:33     ` [PATCH v3 9/9] fixup! remote-curl: error on incomplete packet Denton Liu
2020-05-22 15:54       ` Jeff King
2020-05-22 16:05         ` Denton Liu
2020-05-22 16:31           ` Jeff King

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=20200518171249.GA2462058@generichostname \
    --to=liu.denton@gmail.com \
    --cc=charlieio@outlook.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.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