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

Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods

From
Jeff King <peff@peff.net>
Date
Jul 16, 2016, 07:56 UTC
Message-ID
<20160716075621.GA10275@sigill.intra.peff.net>
In-Reply-To
<CAGZ79kbernZHx9SUG-_vbxy-g77_3ki1uc-21LCviwrH=aXp6w@mail.gmail.com>
On Fri, Jul 15, 2016 at 10:24:16AM -0700, Stefan Beller wrote:
Show 8 quoted lines
> > @@ -319,10 +331,60 @@ static void rp_error(const char *err, ...)
> >  static int copy_to_sideband(int in, int out, void *arg)
> >  {
> >         char data[128];
> 
> While looking at this code, do you think it is feasible to increase the
> size of data[] to 1024 ? (The largest that is possible when
> side-band, but no side-band-64k is given).

I don't see any reason it could not be increased. On the other hand, I find it unlikely to accomplish anything. We are relaying stderr over sideband 2 here, so assuming that typically consists of human-readable lines, they're unlikely to go over 128 (and if they do, they simply get split into two packets).

I suppose if your hooks dump large ascii art to the user, we could save a few framing bytes.

So I don't mind it, but it's definitely orthogonal to this series.
Show 14 quoted lines
> The method was short and concise, this adds a lot of lines.
> Remembering d751dd11 (2016-07-10, hoist out handle_nonblock
> function for xread and xwrite), do you think it would be reasonable to
> put the whole poll handling into a dedicated function, maybe even reuse the
> that function?
> 
>     if (keepalive_active) {
>         if (wrapper_around_poll(&data_in) < 0) // handles EINTR internally
>             break;
>         if (!data_in)
>             send_keep_alive();
>     }
> 
> I am not sure if that makes this function more legible, just food for thought.

That would skip the EINTR continue, though since we're looping anyway here, I'm not sure it's a big deal. The biggest simplification is more like this, I think:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e41f55f..8d792a2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -328,6 +328,32 @@ static void rp_error(const char *err, ...)
 	va_end(params);
 }
 
+/* returns 1 if data is available, 0 otherwise */
+static int keepalive_poll(int in, int out)
+{
+	struct pollfd pfd;
+	int ret;
+
+	pfd.fd = fd;
+	pfd.events = POLLIN;
+	ret = poll(&pfd, 1, 1000 * keepalive_in_sec);
+
+	if (ret < 0) {
+		if (errno == EINTR)
+			return 0; /* no data, we should try again */
+		else
+			return 1; /* no data, but read should return real error */
+	}
+	if (ret == 0) {
+		/* no data; send a keepalive packet */
+		static const char buf[] = "0005\1";
+		write_or_die(out, buf, sizeof(buf) - 1);
+		return 0;
+	}
+	/* else there is actual data to read */
+	return 1;
+}
+
 static int copy_to_sideband(int in, int out, void *arg)
 {
 	char data[128];
@@ -341,26 +367,8 @@ static int copy_to_sideband(int in, int out, void *arg)
 	while (1) {
 		ssize_t sz;
 
-		if (keepalive_active) {
-			struct pollfd pfd;
-			int ret;
-
-			pfd.fd = in;
-			pfd.events = POLLIN;
-			ret = poll(&pfd, 1, 1000 * keepalive_in_sec);
-
-			if (ret < 0) {
-				if (errno == EINTR)
-					continue;
-				else
-					break;
-			} else if (ret == 0) {
-				/* no data; send a keepalive packet */
-				static const char buf[] = "0005\1";
-				write_or_die(1, buf, sizeof(buf) - 1);
-				continue;
-			} /* else there is actual data to read */
-		}
+		if (keepalive_active && !keepalive_poll(in, 1))
+			continue;
 
 		sz = xread(in, data, sizeof(data));
 		if (sz <= 0)

Note that there are actually three outcomes to poll (error, timeout, or
data), and this simplifies away the "error" case to just "try to read
some data" and assumes that the read() will cover (which is similar to
what xread() does).

I dunno if it really helps much, though. There's still a bunch of
keepalive logic in the function to handle the NUL-signal case.

I guess your proposal is more to have an "xpoll" that handles the EINTR
looping, and leave that logic in place. I think that still leaves most
of the complexity in the function.

> > +                       } else if (ret == 0) {
> > +                               /* no data; send a keepalive packet */
> > +                               static const char buf[] = "0005\1";
> 
> and the \1 is the first sideband. Why do we choose that sideband?

What other sideband do you propose? :)

More seriously, this is modeled on the similar keepalive in upload-pack.
The only other option is sideband 2, and IIRC, clients do bad things
with empty band-2 packets (like printing "remote: " but never finishing
the line if we never send any actual data).

> > +                               write_or_die(1, buf, sizeof(buf) - 1);
> > +                               continue;
> > +                       } /* else there is actual data to read */
> 
> "If there is data to read, we need to break the while(1), to actually
> read the data?"

We're not breaking the while; we're falling through to the read call.

> I got confused and needed to go back and read the actual code again,
> would it make sense to rather have a loop here?
> 
>     while (1) {
>        while(keepalive_active) {
>         if (wrapper_around_poll(&data_in) < 0) // handles EINTR internally
>             break;
>         if (!data_in)
>             send_keep_alive();
>         else
>             break;
>         }

This doesn't do the same thing. On poll() error, you are only breaking
out of the inner loop. My intent was to stop copy_to_sideband()
entirely. Perhaps this is the source of confusion; you were thinking of
this more like the loop in xread() where we _want_ to do the final
read() (even though we expect it to be an error!) to give the caller the
correct errno value.

Whereas here, I think it's fine to leave as soon as poll() complains. At
least that was how I wrote it.

I don't think doing it your way is actually wrong, it just wasn't what I
intended.

> > +               if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {
> > +                       const char *p = memchr(data, '\0', sz);
> > +                       if (p) {
> > +                               /*
> > +                                * The NUL tells us to start sending keepalives. Make
> > +                                * sure we send any other data we read along
> > +                                * with it.
> > +                                */
> > +                               keepalive_active = 1;
> > +                               send_sideband(1, 2, data, p - data, use_sideband);
> > +                               send_sideband(1, 2, p + 1, sz - (p - data + 1), use_sideband);
> > +                               continue;
> 
> Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.
> I wonder if we can use a better read function, that would stop reading at a NUL,
> and return early instead?

How would you do that, if not by read()ing a byte at a time (which is
inefficient)? Otherwise you have to deal with the leftovers (after the
NUL) in your buffer. It's one of the reasons I went with a single-byte
signal, because otherwise it gets rather complicated to do robustly.

-Peff
Previous: Stefan BellerNext: Stefan Beller
Message 21 of 26 in “push progress reporting and keepalives”
  1. 0/12 push progress reporting and keepalivesJeff King, Jul 15, 2016
  2. 01/12 check_everything_connected: always pass --quiet to rev-listJeff King, Jul 15, 2016
  3. 02/12 rev-list: add optional progress reportingJeff King, Jul 15, 2016
  4. Junio C HamanoJul 15, 2016
  5. Jeff KingJul 16, 2016
  6. 03/12 check_everything_connected: convert to argv_arrayJeff King, Jul 15, 2016
  7. 04/12 check_everything_connected: use a struct with named optionsJeff King, Jul 15, 2016
  8. Junio C HamanoJul 15, 2016
  9. Jeff KingJul 16, 2016
  10. 05/12 check_connected: relay errors to alternate descriptorJeff King, Jul 15, 2016
  11. Junio C HamanoJul 15, 2016
  12. Jeff KingJul 16, 2016
  13. 06/12 check_connected: add progress flagJeff King, Jul 15, 2016
  14. 07/12 clone: use a real progress meter for connectivity checkJeff King, Jul 15, 2016
  15. 08/12 index-pack: add flag for showing delta-resolution progressJeff King, Jul 15, 2016
  16. 09/12 receive-pack: turn on index-pack resolving progressJeff King, Jul 15, 2016
  17. 10/12 receive-pack: relay connectivity errors to sidebandJeff King, Jul 15, 2016
  18. 11/12 receive-pack: turn on connectivity progressJeff King, Jul 15, 2016
  19. 12/12 receive-pack: send keepalives during quiet periodsJeff King, Jul 15, 2016
  20. Stefan BellerJul 15, 2016
  21. Jeff KingJul 16, 2016
  22. Stefan BellerJul 19, 2016
  23. Jeff KingJul 19, 2016
  24. Stefan BellerJul 19, 2016
  25. Jeff KingJul 20, 2016
  26. Junio C HamanoJul 15, 2016

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.