[PATCH 6 of 6] bundle2: don't attempt I/O after an I/O error occurs (issue4784)

Gregory Szorc gregory.szorc at gmail.com
Sun Apr 16 15:00:08 EDT 2017


On Sat, Apr 15, 2017 at 8:41 PM, Yuya Nishihara <yuya at tcha.org> wrote:

> On Fri, 14 Apr 2017 00:44:10 -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc at gmail.com>
> > # Date 1492155656 25200
> > #      Fri Apr 14 00:40:56 2017 -0700
> > # Node ID d1b31c378dc8e29d9827f9ded9fd023d5a00b0c9
> > # Parent  8525abda8397f2a5a94edc5db2279549ef53b8e8
> > bundle2: don't attempt I/O after an I/O error occurs (issue4784)
>
> > A subtle bug with this change is that *all* I/O errors will influence
> > behavior, not just I/O errors on the bundle2 stream. e.g. an I/O error
> > from the local filesystem could result in not seeking the bundle. This
> > is the source of the test-acl.t test change, for example. I question
> > whether we care. Considering pretty much all bundle2 stream I/O errors
> > resulted in this code not running before (due to a fast I/O error
> > re-raise), we seem to be getting along just fine without the code. And
> > since this error recovery code is only tested in test-acl.t, I'm
> > guessing it isn't important. Or, at least it isn't as important as
> > accurately reporting the original I/O error. Considering these "stream
> > ended unexpectedly" errors have been masking several problems, I
> > think the new code is acceptable. We can always improve the stream
> > error detection code later if the new model isn't good enough.
>
> > diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> > --- a/mercurial/bundle2.py
> > +++ b/mercurial/bundle2.py
> > @@ -354,9 +354,13 @@ def processbundle(repo, unbundler, trans
> >          for nbpart, part in iterparts:
> >              _processpart(op, part)
> >      except Exception as exc:
> > -        for nbpart, part in iterparts:
> > -            # consume the bundle content
> > -            part.seek(0, 2)
> > +        # Don't attempt further I/O after we've encountered an I/O
> failure.
> > +        # This will almost certainly just raise another I/O failure and
> will
> > +        # only mask the underlying failure.
> > +        if not isinstance(exc, (IOError, error.RichIOError)):
> > +            for nbpart, part in iterparts:
> > +                # consume the bundle content
> > +                part.seek(0, 2)
>
> I think this is okay, but if we have to care about a local IOError, maybe
> we
> could try consuming bundle and suppress errors?
>
>   except Exception as exc:
>       try:
>           # consume the bundle content
>       except (IOError, error.RichIOError):
>           pass
>

I went with something similar to this in the series I just sent. But it is
different enough that we may want to discuss it further.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170416/dc91a3d6/attachment.html>


More information about the Mercurial-devel mailing list