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

[Stefan Beller <sbeller@google.com>]

On Tue, Jul 19, 2016 at 3:07 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Jul 18, 2016 at 10:28:25PM -0700, Stefan Beller wrote:
>
>> On Sat, Jul 16, 2016 at 12:56 AM, Jeff King <peff@peff.net> wrote:
>> >> > +               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