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

On Tue, Jul 19, 2016 at 3:07 AM, Jeff King [off-list ref] wrote:

On Mon, Jul 18, 2016 at 10:28:25PM -0700, Stefan Beller wrote:

quoted

On Sat, Jul 16, 2016 at 12:56 AM, Jeff King [off-list ref] wrote:

quoted

quoted

quoted

+               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.

I do not question the concept of a single NUL byte, but rather the
implementation, i.e. if we had an xread_until_nul you would not need
to have a double send_sideband here?

What would xread_until_nul() look like?

The only reasonable implementation I can think of is:

  ssize_t xread_until_nul(int fd, char *out, size_t len)
  {
        ssize_t total = 0;
        while (total < len) {
                ssize_t ret = xread(fd, out + total, 1);
                if (ret < 0) {
                        /* Oops, anything in out[0..total] is lost, but
                         * we have no way of signaling both a partial
                         * read and an error at the end; fixable by
                         * changing the interface, but doesn't really
                         * matter in practice for this application. */
                        return -1;
                }
                if (ret == 0)
                        break; /* EOF, stop reading */
                if (out[total] == '\0')
                        break; /* got our NUL, stop reading */
                total++;
        }
        return total;
  }

There are two problems with this function:

  1. Until we see the NUL, we're making an excessive number of read()
     syscalls looking for it. You could make larger reads, but then what
     do you do with the residual bytes (i.e., the ones after the NUL in
     the buffer you read)? You'd have to somehow save it to return at
     the next xread_until_nul(). Which implie some kind of internal
     buffering.

     The reason there are two send_sidebands is to cover the case where
     we have some real data, then the signal byte, then some more data.
     So instead of buffering, we just handle the data immediately.

  2. How does it know when to return?

     We want to send the data as soon as it is available, in as large a
     chunk as possible. Using a single xread() as we do now, we get
     whatever the OS has for us, up to our buffer size.

     But after each 1-byte read above, we would not want to return;
     there might be more data. So it keeps reading until NUL or we fill
     our buffer. But that may mean the xread() blocks, even though we
     have data that _could_ be shipped over the sideband.

     To fix that, you'd have to poll() for each xread(), and return when
     it says nothing's ready.

I know that writing the function myself and then critiquing is the very
definition of a strawman. :) But it's the best I could think of.  Maybe
you had something more clever in mind?

Actually no, I had not. I was hoping you could come up with a clever thing.
My original point was the perceived added complexity to a simple
seemingly simple function (copy_to_sideband in your original patch),
so my gut reaction was to shove the complexity away into a helper function.
But no matter how it is done, there is always this one function that looks
complex for this problem. So I think your original approach is fine then?

Thanks,
Stefan

-Peff