Merge lp:~vila/bzr/788530-better-invalid-http-response into lp:bzr

[Vincent Ladeuil]

Don't swallow the original error when reporting an InvalidHttpResponse.

This tweaks InvalidHttpResponse so the orig_error parameter is not swallowed anymore.

This exception class is generally subclassed when a more precise reporting is desired and used as a catch-all for the other cases.

The catual implementation has the nasty side-effect of swallowing the original error making the debugging harder.

I branched lp:bzr/2.3 just in case we want to backport it (feedback welcome, I'm targeting trunk by default) and I'll add a news entry in the right file if/when approved.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/26/2011 12:02 PM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/788530-better-invalid-http-response into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #788530 in Bazaar: "InvalidHttpResponse swallows the original error making debugging harder"
> https://bugs.launchpad.net/bzr/+bug/788530
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/788530-better-invalid-http-response/+merge/62443
>
> This tweaks InvalidHttpResponse so the orig_error parameter is not swallowed anymore.
>
> This exception class is generally subclassed when a more precise reporting is desired and used as a catch-all for the other cases.
>
> The catual implementation has the nasty side-effect of swallowing the original error making the debugging harder.
>
> I branched lp:bzr/2.3 just in case we want to backport it (feedback welcome, I'm targeting trunk by default) and I'll add a news entry in the right file if/when approved.

I would put the bug # in the commit message. Other than that this seems
fine.

I'm not sure that the special cases always call up the stack, and
certainly they need to do their own formatting for "orig_error". My only
concern is double-handling. (TransportError itself has an 'if orig_error
is None: block, though it just uses a plain str.)
Specifically, this adds ': %r' % (orig_error,) a child class could
easily do the same, and you get the repr of a string of a repr of the
original exception, etc.

For now, though, it seems fine. We usually add tests into
bzrlib/tests/test_errors.py for formatting stuff like this, though.

John
=:->
  merge: approve

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3eJwYACgkQJdeBCYSNAAMoKACeINxJf7SdBnjA9Pwp8L2lrZLq
0FoAoIxETGBhnrtW7H7fWkEYma4qiJbC
=s/uO
-----END PGP SIGNATURE-----

review: Approve

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

<snip/>

    > I would put the bug # in the commit message. Other than that this seems
    > fine.

    > I'm not sure that the special cases always call up the stack, and
    > certainly they need to do their own formatting for "orig_error". My only
    > concern is double-handling. (TransportError itself has an 'if orig_error
    > is None: block, though it just uses a plain str.)

Which is overridden there.

    > Specifically, this adds ': %r' % (orig_error,) a child class could
    > easily do the same, and you get the repr of a string of a repr of the
    > original exception, etc.

The existing daughter classes already either define a specific _fmt and/or
don't accept orig_error as a parameter so aren't concerned by this fix.

    > For now, though, it seems fine. We usually add tests into
    > bzrlib/tests/test_errors.py for formatting stuff like this, though.

For use facing errors that's true, in this specific case I'm addressing
debug concerns, we should not use InvalidHttpResponse for user-facing
errors.

I didn't add a test as I suspect different versions of python to
possibly introduce some slight variations there and I avoided %s and
str(exception) as we had unicode issues in the past around such cases
(unlikely at this protocol level but the case at hand was a bogus server
suddenly stopping to speak HTTP so who knows what garbage we may get
there). Using %r may not be enough but it's a safer first step.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

> The existing daughter classes already either define a specific _fmt and/or
> don't accept orig_error as a parameter so aren't concerned by this fix.
>
> > For now, though, it seems fine. We usually add tests into
> > bzrlib/tests/test_errors.py for formatting stuff like this, though.
>
> For use facing errors that's true, in this specific case I'm addressing
> debug concerns, we should not use InvalidHttpResponse for user-facing
> errors.

I thought the whole point of the original bug was that this got shown as
a *user-facing* error, and that it made it confusing what the original
failure was.

>
> I didn't add a test as I suspect different versions of python to
> possibly introduce some slight variations there and I avoided %s and
> str(exception) as we had unicode issues in the past around such cases
> (unlikely at this protocol level but the case at hand was a bogus server
> suddenly stopping to speak HTTP so who knows what garbage we may get
> there). Using %r may not be enough but it's a safer first step.
>

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3eLg4ACgkQJdeBCYSNAAP4YwCeIDvYyDX0U3XZ2RKx5Q9OxkJh
sF8An1KqdEI+o1pFy16PNZ7An1BuxPSo
=TnRu
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

    > ...

    >> The existing daughter classes already either define a specific _fmt and/or
    >> don't accept orig_error as a parameter so aren't concerned by this fix.
    >>
    >> > For now, though, it seems fine. We usually add tests into
    >> > bzrlib/tests/test_errors.py for formatting stuff like this, though.
    >>
    >> For use facing errors that's true, in this specific case I'm addressing
    >> debug concerns, we should not use InvalidHttpResponse for user-facing
    >> errors.

    > I thought the whole point of the original bug was that this got shown as
    > a *user-facing* error, and that it made it confusing what the original
    > failure was.

Well, it's an unexpected exception: and invalid http response, this is
not some kind of error we expect and for which we can give a better
explanation to the user, it's used as a catch-all for several kind of
low-level errors.

[Martin Packman (gz)]

Good change. Really bzrlib.errors could do with a uniform idiom for including wrapped errors as a number of exception classes want to do it.

+ orig_error = ': %r' % (orig_error,)

This is fine in Python 2.5 and later where the repr generally includes the details you care about, in 2.4 most exceptions just have object.__repr__ which is less useful. The nice thing about %r over %s is it avoids the bug with localised error messages with non-ascii characters.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/26/2011 06:43 PM, Martin [gz] wrote:
> Good change. Really bzrlib.errors could do with a uniform idiom for including wrapped errors as a number of exception classes want to do it.
>
> + orig_error = ': %r' % (orig_error,)
>
> This is fine in Python 2.5 and later where the repr generally includes the details you care about, in 2.4 most exceptions just have object.__repr__ which is less useful. The nice thing about %r over %s is it avoids the bug with localised error messages with non-ascii characters.

Ah, but this is landing in bzr-2.4 so we don't care :).

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3erFgACgkQJdeBCYSNAAPbuQCdENg6T+ioPdY4uPO+Yylw1hqQ
n0QAn1NhS2ODw5Rfc2p02BW792a0kn22
=lY4M
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

> Good change. Really bzrlib.errors could do with a uniform idiom for including
> wrapped errors as a number of exception classes want to do it.
>
> + orig_error = ': %r' % (orig_error,)
>
> This is fine in Python 2.5 and later where the repr generally includes the
> details you care about, in 2.4 most exceptions just have object.__repr__ which
> is less useful. The nice thing about %r over %s is it avoids the bug with
> localised error messages with non-ascii characters.

Thanks for the precisions. I won't land it in 2.3 then.

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2011-05-26 08:05:45 +0000
+++ bzrlib/errors.py	2011-05-26 20:28:54 +0000
@@ -1715,10 +1715,16 @@
 
 class InvalidHttpResponse(TransportError):
 
-    _fmt = "Invalid http response for %(path)s: %(msg)s"
+    _fmt = "Invalid http response for %(path)s: %(msg)s%(orig_error)s"
 
     def __init__(self, path, msg, orig_error=None):
         self.path = path
+        if orig_error is None:
+            orig_error = ''
+        else:
+            # This is reached for obscure and unusual errors so we want to
+            # preserve as much info as possible to ease debug.
+            orig_error = ': %r' % (orig_error,)
         TransportError.__init__(self, msg, orig_error=orig_error)
 
 

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-05-26 16:28:31 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-05-26 20:28:54 +0000
@@ -32,6 +32,9 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Reports the original error when an InvalidHttpResponse exception is
+  encountered to facilitate debug. (Vincent Ladeuil, #788530)
+
 Documentation
 *************