git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 7/7] stateless-connect: send response end packet

From
Jeff King <peff@peff.net>
Date
May 18, 2020, 17:26 UTC
Message-ID
<20200518172605.GA52384@coredump.intra.peff.net>
In-Reply-To
<20200518171249.GA2462058@generichostname>
On Mon, May 18, 2020 at 01:12:49PM -0400, Denton Liu wrote:
Show 29 quoted lines
> > 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.

I think you're right that it does, but TBH I find it harder to follow, because now we're even further away from the actual response reads. What I most want to verify is:

  - there is no response-reading code path that does not check the
    delimiters
  - there is no code path where we check delimiters but did not actually
    read a response

So to me, putting the check as close to the response-reads as possible makes sense. I think the patch below is equivalent and easier to verify, but I admit it's somewhat subjective:

diff --git a/fetch-pack.c b/fetch-pack.c
index 5325649eda..0bf1e5760c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1443,6 +1443,17 @@ static void receive_wanted_refs(struct packet_reader *reader,
 		die(_("error processing wanted refs: %d"), reader->status);
 }
 
+static void check_stateless_delimiter(struct fetch_pack_args *args,
+				      struct packet_reader *reader)
+{
+	if (!args->stateless_rpc)
+		return; /* not in stateless mode, no delimiter expected */
+	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"));
+}
+
 enum fetch_state {
 	FETCH_CHECK_LOCAL = 0,
 	FETCH_SEND_REQUEST,
@@ -1469,7 +1480,6 @@ 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;
 
 	if (args->no_dependents) {
 		negotiator = NULL;
@@ -1488,8 +1498,6 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	}
 
 	while (state != FETCH_DONE) {
-		check_http_delimiter = 0;
-
 		switch (state) {
 		case FETCH_CHECK_LOCAL:
 			sort_ref_list(&ref, ref_compare_name);
@@ -1538,15 +1546,19 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 			/* Process ACKs/NAKs */
 			switch (process_acks(negotiator, &reader, &common)) {
 			case READY:
+				/*
+				 * Don't check for response delimiter; get_pack() will
+				 * read the rest of this response.
+				 */
 				state = FETCH_GET_PACK;
 				break;
 			case COMMON_FOUND:
 				in_vain = 0;
 				seen_ack = 1;
 				/* fallthrough */
 			case NO_COMMON_FOUND:
 				state = FETCH_SEND_REQUEST;
-				check_http_delimiter = 1;
+				check_stateless_delimiter(args, &reader);
 				break;
 			}
 			break;
@@ -1565,20 +1577,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 			process_section_header(&reader, "packfile", 0);
 			if (get_pack(args, fd, pack_lockfile, sought, nr_sought))
 				die(_("git fetch-pack: fetch failed."));
+			check_stateless_delimiter(args, &reader);
 
 			state = FETCH_DONE;
-			check_http_delimiter = 1;
 			break;
 		case FETCH_DONE:
 			continue;
 		}
-
-		if (args->stateless_rpc && check_http_delimiter) {
-			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"));
-		}
 	}
 
 	if (negotiator)

> > 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 ;)

OK, I didn't realize we had explicitly marked it as experimental. I
agree we're probably fine here, then (now that v2 is starting to settle
a bit more, we might think about lifting that warning, but this patch
series shows we're not quite there yet :) ).

-Peff
Previous: Denton LiuNext: Denton Liu
Message 37 of 62 in “remote-curl: partial fix for a deadlock with stateless rpc”
  1. 0/6 remote-curl: partial fix for a deadlock with stateless rpcDenton Liu, May 13, 2020
  2. 1/6 remote-curl: fix typoDenton Liu, May 13, 2020
  3. 2/6 remote-curl: remove label indentationDenton Liu, May 13, 2020
  4. 3/6 transport: combine common cases with a fallthroughDenton Liu, May 13, 2020
  5. Eric SunshineMay 13, 2020
  6. Denton LiuMay 18, 2020
  7. Eric SunshineMay 18, 2020
  8. 4/6 pkt-line: extern packet_length()Denton Liu, May 13, 2020
  9. Eric SunshineMay 13, 2020
  10. Jeff KingMay 15, 2020
  11. Jeff KingMay 15, 2020
  12. 5/6 remote-curl: error on incomplete packetDenton Liu, May 13, 2020
  13. Jeff KingMay 15, 2020
  14. Denton LiuMay 18, 2020
  15. Jeff KingMay 18, 2020
  16. 6/6 remote-curl: ensure last packet is a flushDenton Liu, May 13, 2020
  17. Denton LiuMay 15, 2020
  18. Jeff KingMay 15, 2020
  19. Junio C HamanoMay 18, 2020
  20. Jeff KingMay 18, 2020
  21. Jeff KingMay 18, 2020
  22. 0/7 remote-curl: fix deadlocks when remote server disconnectsDenton Liu, May 18, 2020
  23. 1/7 remote-curl: fix typoDenton Liu, May 18, 2020
  24. 2/7 remote-curl: remove label indentationDenton Liu, May 18, 2020
  25. Junio C HamanoMay 18, 2020
  26. 4/7 pkt-line: extern packet_length()Denton Liu, May 18, 2020
  27. Jeff KingMay 18, 2020
  28. Eric SunshineMay 18, 2020
  29. Jeff KingMay 18, 2020
  30. Junio C HamanoMay 18, 2020
  31. 3/7 transport: extract common fetch_pack() callDenton Liu, May 18, 2020
  32. Junio C HamanoMay 18, 2020
  33. 6/7 pkt-line: PACKET_READ_RESPONSE_ENDDenton Liu, May 18, 2020
  34. 7/7 stateless-connect: send response end packetDenton Liu, May 18, 2020
  35. Jeff KingMay 18, 2020
  36. Denton LiuMay 18, 2020
  37. Jeff KingMay 18, 2020
  38. 5/7 remote-curl: error on incomplete packetDenton Liu, May 18, 2020
  39. Jeff KingMay 18, 2020
  40. Denton LiuMay 18, 2020
  41. Jeff KingMay 18, 2020
  42. Denton LiuMay 18, 2020
  43. Jeff KingMay 18, 2020
  44. Junio C HamanoMay 18, 2020
  45. Jeff KingMay 19, 2020
  46. Junio C HamanoMay 18, 2020
  47. 0/7 remote-curl: fix deadlocks when remote server disconnectsDenton Liu, May 19, 2020
  48. 1/7 remote-curl: fix typoDenton Liu, May 19, 2020
  49. 2/7 remote-curl: remove label indentationDenton Liu, May 19, 2020
  50. 3/7 transport: extract common fetch_pack() callDenton Liu, May 19, 2020
  51. 4/7 pkt-line: extern packet_length()Denton Liu, May 19, 2020
  52. Eric SunshineMay 19, 2020
  53. 6/7 pkt-line: define PACKET_READ_RESPONSE_ENDDenton Liu, May 19, 2020
  54. 5/7 remote-curl: error on incomplete packetDenton Liu, May 19, 2020
  55. 7/7 stateless-connect: send response end packetDenton Liu, May 19, 2020
  56. Jeff KingMay 19, 2020
  57. Denton LiuMay 19, 2020
  58. 8/7 fixup! pkt-line: extern packet_length()Denton Liu, May 19, 2020
  59. 9/9 fixup! remote-curl: error on incomplete packetDenton Liu, May 22, 2020
  60. Jeff KingMay 22, 2020
  61. Denton LiuMay 22, 2020
  62. Jeff KingMay 22, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.