[PATCH STABLE] changegroup: retry read() when partial data returned

Gregory Szorc gregory.szorc at gmail.com
Sun Mar 26 17:26:11 EDT 2017


On Sun, Mar 26, 2017 at 12:17 PM, Jun Wu <quark at fb.com> wrote:

> The direction looks good. I'd like to see more solid code. Commented below.
>
> Excerpts from Gregory Szorc's message of 2017-03-26 11:56:59 -0700:
> > +    # Retry to get remaining data. In cases where the stream has
> errored or
> > +    # is at EOF, this will do a bit of redundant work. But it helps
> prevent
> > +    # intermittent failures and isn't common. So the overhead is
> acceptable.
> > +    for i in range(3):
>
> If "stream.read" does not behave as expected, assume it's totally broken.
> So we need to reset the retry counter once we got data. Something like:
>
>     def readexactly(..., retry=3):
>         ....
>         retryremaining = retry
>         while True:
>             chunk = stream.read(left)
>             if not chunk:
>                 retryremaining -= 1
>                 if not retryremaining:
>                     break
>             retryremaining = retry
>             left -= len(chunk)
>             ....
>

I agree with your reasoning here. However, I want to be conservative with
the patch since it is for stable. If a reviewer finds your approach
suitable for stable, I'll make the change. Otherwise, we can take the
conservative approach on stable and change to your version on default.

FWIW there are other things I may want to do on default, such as making the
retry count configurable and possibly adding some blackbox logging to help
us track down how this fails in the wild.

> +        chunk = stream.read(left)
> > +        chunks.append(chunk)
> > +        left -= len(chunk)
> > +        assert left >= 0
> > +
> > +        if not left:
> > +            break
> > +
> > +    if left:
> >          raise error.Abort(_("stream ended unexpectedly"
> >                             " (got %d bytes, expected %d)")
> > -                          % (len(s), n))
> > -    return s
> > +                          % (n - left, n))
> > +
> > +    return b''.join(chunks)
> >
> >  def getchunk(stream):
> >      """return the next chunk from stream as a string"""
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170326/6fedaf15/attachment.html>


More information about the Mercurial-devel mailing list