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

Yuya Nishihara yuya at tcha.org
Sat Apr 15 23:41:00 EDT 2017


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


More information about the Mercurial-devel mailing list