Patchwork [3,of,3] bundle2: ignore errors seeking a bundle after an exception (issue4784)

login
register
mail settings
Submitter Gregory Szorc
Date April 16, 2017, 6:58 p.m.
Message ID <9ab4130a8302b32f7c86.1492369089@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/20242/
State Accepted
Headers show

Comments

Gregory Szorc - April 16, 2017, 6:58 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1492368908 25200
#      Sun Apr 16 11:55:08 2017 -0700
# Node ID 9ab4130a8302b32f7c86171a669e2ea2bfdd496b
# Parent  420b7094137e851132849b3cc8ddecf255a09bcb
bundle2: ignore errors seeking a bundle after an exception (issue4784)

Many have seen a "stream ended unexpectedly" error. This message is
raised from changegroup.readexactly() when a read(n) operation fails
to return exactly N bytes.

I believe most occurrences of this error in the wild stem from
the code changed in this patch. Before, if bundle2's part applicator
raised an Exception when processing/applying parts, the exception
handler would attempt to iterate the remaining parts. If I/O
during this iteration failed, it would likely raise the
"stream ended unexpectedly" exception.

The problem with this approach is that if we already encountered
an I/O error iterating the bundle2 data during application, then
any further I/O would almost certainly fail. If the original stream
were closed, changegroup.readexactly() would obtain an empty
string, triggering "stream ended unexpectedly" with "got 0." This is
the error message that users would see. What's worse is that the
original I/O related exception would be lost since a new exception
would be raised. This made debugging the actual I/O failure
effectively impossible.

This patch changes the exception handler for bundle2 application to
ignore errors when seeking the underlying stream. When the underlying
error is I/O related, the seek should fail fast and the original
exception will be re-raised. The output changes in
test-http-bad-server.t demonstrate this.

When the underlying error is not I/O related and the stream can be
seeked, the old behavior is preserved.
Gregory Szorc - April 16, 2017, 7:03 p.m.
On Sun, Apr 16, 2017 at 11:58 AM, Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1492368908 25200
> #      Sun Apr 16 11:55:08 2017 -0700
> # Node ID 9ab4130a8302b32f7c86171a669e2ea2bfdd496b
> # Parent  420b7094137e851132849b3cc8ddecf255a09bcb
> bundle2: ignore errors seeking a bundle after an exception (issue4784)
>
> Many have seen a "stream ended unexpectedly" error. This message is
> raised from changegroup.readexactly() when a read(n) operation fails
> to return exactly N bytes.
>
> I believe most occurrences of this error in the wild stem from
> the code changed in this patch. Before, if bundle2's part applicator
> raised an Exception when processing/applying parts, the exception
> handler would attempt to iterate the remaining parts. If I/O
> during this iteration failed, it would likely raise the
> "stream ended unexpectedly" exception.
>
> The problem with this approach is that if we already encountered
> an I/O error iterating the bundle2 data during application, then
> any further I/O would almost certainly fail. If the original stream
> were closed, changegroup.readexactly() would obtain an empty
> string, triggering "stream ended unexpectedly" with "got 0." This is
> the error message that users would see. What's worse is that the
> original I/O related exception would be lost since a new exception
> would be raised. This made debugging the actual I/O failure
> effectively impossible.
>
> This patch changes the exception handler for bundle2 application to
> ignore errors when seeking the underlying stream. When the underlying
> error is I/O related, the seek should fail fast and the original
> exception will be re-raised. The output changes in
> test-http-bad-server.t demonstrate this.
>
> When the underlying error is not I/O related and the stream can be
> seeked, the old behavior is preserved.
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -354,9 +354,19 @@ 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)
> +        # Any exceptions seeking to the end of the bundle at this point
> are
> +        # almost certainly related to the underlying stream being bad.
> +        # And, chances are that the exception we're handling is related to
> +        # getting in that bad state. So, we swallow the seeking error and
> +        # re-raise the original error.
> +        seekerror = False
> +        try:
> +            for nbpart, part in iterparts:
> +                # consume the bundle content
> +                part.seek(0, 2)
> +        except Exception:
> +            seekerror = True
>
>
I'm not super thrilled about swallowing the inner exception. We could raise
a special exception type that can represent multiple errors and have the
error format being something like "in addition, an error occurred when
seeking to the end of the bundle: %s." We could exclude IOError and
PeerTransportError from reporting the inner exception, possibly only if the
original error is one of these.

I'm not sure what logic is best here. But as this patch stands, we report
the original error in all cases, which feels strictly better than what we
did before. The question is whether the inner exception holds any value and
is worth reporting.


>          # Small hack to let caller code distinguish exceptions from
> bundle2
>          # processing from processing the old format. This is mostly
>          # needed to handle different return codes to unbundle according
> to the
> @@ -370,7 +380,13 @@ def processbundle(repo, unbundler, trans
>              replycaps = op.reply.capabilities
>          exc._replycaps = replycaps
>          exc._bundle2salvagedoutput = salvaged
> -        raise
> +
> +        # Re-raising from a variable loses the original stack. So only use
> +        # that form if we need to.
> +        if seekerror:
> +            raise exc
> +        else:
> +            raise
>      finally:
>          repo.ui.debug('bundle2-input-bundle: %i parts total\n' % nbpart)
>
> diff --git a/tests/test-http-bad-server.t b/tests/test-http-bad-server.t
> --- a/tests/test-http-bad-server.t
> +++ b/tests/test-http-bad-server.t
> @@ -686,7 +686,8 @@ Server stops sending after bundle2 part
>    adding changesets
>    transaction abort!
>    rollback completed
> -  abort: stream ended unexpectedly (got 0 bytes, expected 4)
> +  abort: HTTP request error (incomplete response)
> +  (this may be an intermittent network failure; if the error persists,
> consider contacting the network or server operator)
>    [255]
>
>    $ killdaemons.py $DAEMON_PIDS
> @@ -715,7 +716,8 @@ Server stops after bundle2 part payload
>    adding changesets
>    transaction abort!
>    rollback completed
> -  abort: stream ended unexpectedly (got 0 bytes, expected 4)
> +  abort: HTTP request error (incomplete response)
> +  (this may be an intermittent network failure; if the error persists,
> consider contacting the network or server operator)
>    [255]
>
>    $ killdaemons.py $DAEMON_PIDS
> @@ -745,7 +747,8 @@ Server stops sending in middle of bundle
>    adding changesets
>    transaction abort!
>    rollback completed
> -  abort: stream ended unexpectedly (got 0 bytes, expected 4)
> +  abort: HTTP request error (incomplete response)
> +  (this may be an intermittent network failure; if the error persists,
> consider contacting the network or server operator)
>    [255]
>
>    $ killdaemons.py $DAEMON_PIDS
>
Yuya Nishihara - April 17, 2017, 1:51 p.m.
On Sun, 16 Apr 2017 12:03:25 -0700, Gregory Szorc wrote:
> On Sun, Apr 16, 2017 at 11:58 AM, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1492368908 25200
> > #      Sun Apr 16 11:55:08 2017 -0700
> > # Node ID 9ab4130a8302b32f7c86171a669e2ea2bfdd496b
> > # Parent  420b7094137e851132849b3cc8ddecf255a09bcb
> > bundle2: ignore errors seeking a bundle after an exception (issue4784)

> > diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> > --- a/mercurial/bundle2.py
> > +++ b/mercurial/bundle2.py
> > @@ -354,9 +354,19 @@ 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)
> > +        # Any exceptions seeking to the end of the bundle at this point
> > are
> > +        # almost certainly related to the underlying stream being bad.
> > +        # And, chances are that the exception we're handling is related to
> > +        # getting in that bad state. So, we swallow the seeking error and
> > +        # re-raise the original error.
> > +        seekerror = False
> > +        try:
> > +            for nbpart, part in iterparts:
> > +                # consume the bundle content
> > +                part.seek(0, 2)
> > +        except Exception:

Nit: I prefer not swallowing Exception which may shadow trivial bugs where
NameError, AttributeError, etc. would be raised.

> > +            seekerror = True
> >
> I'm not super thrilled about swallowing the inner exception. We could raise
> a special exception type that can represent multiple errors and have the
> error format being something like "in addition, an error occurred when
> seeking to the end of the bundle: %s." We could exclude IOError and
> PeerTransportError from reporting the inner exception, possibly only if the
> original error is one of these.

Perhaps.

> I'm not sure what logic is best here. But as this patch stands, we report
> the original error in all cases, which feels strictly better than what we
> did before. The question is whether the inner exception holds any value and
> is worth reporting.

Yeah, I agree.

> > +        # Re-raising from a variable loses the original stack. So only use
> > +        # that form if we need to.
> > +        if seekerror:
> > +            raise exc
> > +        else:
> > +            raise

We could save the original exception by sys.exc_info() and re-raise it, but
Python 3 made that slightly difficult.

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -354,9 +354,19 @@  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)
+        # Any exceptions seeking to the end of the bundle at this point are
+        # almost certainly related to the underlying stream being bad.
+        # And, chances are that the exception we're handling is related to
+        # getting in that bad state. So, we swallow the seeking error and
+        # re-raise the original error.
+        seekerror = False
+        try:
+            for nbpart, part in iterparts:
+                # consume the bundle content
+                part.seek(0, 2)
+        except Exception:
+            seekerror = True
+
         # Small hack to let caller code distinguish exceptions from bundle2
         # processing from processing the old format. This is mostly
         # needed to handle different return codes to unbundle according to the
@@ -370,7 +380,13 @@  def processbundle(repo, unbundler, trans
             replycaps = op.reply.capabilities
         exc._replycaps = replycaps
         exc._bundle2salvagedoutput = salvaged
-        raise
+
+        # Re-raising from a variable loses the original stack. So only use
+        # that form if we need to.
+        if seekerror:
+            raise exc
+        else:
+            raise
     finally:
         repo.ui.debug('bundle2-input-bundle: %i parts total\n' % nbpart)
 
diff --git a/tests/test-http-bad-server.t b/tests/test-http-bad-server.t
--- a/tests/test-http-bad-server.t
+++ b/tests/test-http-bad-server.t
@@ -686,7 +686,8 @@  Server stops sending after bundle2 part 
   adding changesets
   transaction abort!
   rollback completed
-  abort: stream ended unexpectedly (got 0 bytes, expected 4)
+  abort: HTTP request error (incomplete response)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
   [255]
 
   $ killdaemons.py $DAEMON_PIDS
@@ -715,7 +716,8 @@  Server stops after bundle2 part payload 
   adding changesets
   transaction abort!
   rollback completed
-  abort: stream ended unexpectedly (got 0 bytes, expected 4)
+  abort: HTTP request error (incomplete response)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
   [255]
 
   $ killdaemons.py $DAEMON_PIDS
@@ -745,7 +747,8 @@  Server stops sending in middle of bundle
   adding changesets
   transaction abort!
   rollback completed
-  abort: stream ended unexpectedly (got 0 bytes, expected 4)
+  abort: HTTP request error (incomplete response)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
   [255]
 
   $ killdaemons.py $DAEMON_PIDS