classification
Title: implement PEP 3134 exception reporting
Type: enhancement Stage:
Components: Interpreter Core Versions: Python 3.0
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: benjamin.peterson Nosy List: Rhamphoryncus, amaury.forgeotdarc, benjamin.peterson, gvanrossum, pitrou
Priority: critical Keywords: patch

Created on 2008-06-14 20:30 by benjamin.peterson, last changed 2008-07-15 15:32 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
exc_reporting.patch pitrou, 2008-06-18 22:24
exc_reporting.patch pitrou, 2008-06-27 19:13
exc_reporting2.patch pitrou, 2008-07-13 20:45
Messages (28)
msg68214 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-06-14 20:30
Traceback reporting needs to be altered to support exception chaining as
per PEP 3134.
msg68217 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-14 20:46
Thinking about this, I realized that an exception can become its own
context if it is explicitly re-raised in its except handler (with "raise
variable", not bare "raise").
Not a critical bug, but it should be fixed in order to avoid delayed
garbage collection of such exceptions.

(as for the exception reporting code, it will have to detect recursivity)

>>> try: 1/0
... except Exception as e: raise e
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 1, in <module>
ZeroDivisionError: int division or modulo by zero
>>> e = sys.last_value
>>> e.__context__
ZeroDivisionError('int division or modulo by zero',)
>>> e.__context__ is e
True
msg68370 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-18 18:43
Does anyone know why there is the following test in pythonrun.c:
http://hg.pitrou.net/public/py3k/py3k/file/c143699d8dee/Python/pythonrun.c#l1346

Can PyErr_Display be called with something else than a PyException instance?
msg68375 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-18 21:14
Two other questions:

1) Should I expose a PyErr_DisplaySingle API to display an exception
without chaining?

2) Should PyErr_Display return an integer value (0: success, -1:
failure) rather than void as it currently does?
msg68380 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-18 22:24
Here is a draft patch for those who want to take a look.

(it works but the final cleanup is waiting for the API decisions
mentioned above)
msg68427 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-19 22:35
Yet another question. There is a slight discrepancy between tracebacks
generated by the builtin-reporting and tracebacks generated by traceback.py.

With built-in reporting:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "f.py", line 22, in raise_cause
inner_raise_cause()
  File "f.py", line 13, in inner_raise_cause
raise KeyError from e
KeyError

With traceback.py:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "f.py", line 22, in raise_cause
    inner_raise_cause()
  File "f.py", line 13, in inner_raise_cause
    raise KeyError from e
KeyError

As you see, indentation of code lines is different. Should we harmonize
those behaviours?
msg68559 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-22 07:04
* cause/context cycles should be avoided.  Naive traceback printing
could become confused, and I can't think of any accidental way to
provoke it (besides the problem mentioned here.)

* I suspect PyErr_Display handled string exceptions in 2.x, and this is
an artifact of that

* No opinion on PyErr_DisplaySingle

* PyErr_Display is used by PyErr_Print, and it must end up with no
active exception.  Additionally, third party code may depend on this
semantic.  Maybe PyErr_DisplayEx?

* +1 on standardizing tracebacks
msg68565 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-22 14:07
Le dimanche 22 juin 2008 à 07:04 +0000, Adam Olsen a écrit :
> Adam Olsen <rhamph@gmail.com> added the comment:
> 
> * cause/context cycles should be avoided.  Naive traceback printing
> could become confused, and I can't think of any accidental way to
> provoke it (besides the problem mentioned here.)

You mean they should be detected when the exception is set? I was afraid
that it may make exception raising slower. Reporting is not performance
sensitive in comparison to exception raising.

(the "problem mentioned here" is already avoided in the patch, but the
detection of other cycles is deferred to exception reporting for the
reason given above)

> * PyErr_Display is used by PyErr_Print, and it must end up with no
> active exception.  Additionally, third party code may depend on this
> semantic.  Maybe PyErr_DisplayEx?

I was not proposing to change the exception swallowing semantics, just
to add a return value indicating if any errors had occurred while
displaying the exception.
msg68569 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-22 17:17
On Sun, Jun 22, 2008 at 8:07 AM, Antoine Pitrou <report@bugs.python.org> wrote:
> You mean they should be detected when the exception is set? I was afraid
> that it may make exception raising slower. Reporting is not performance
> sensitive in comparison to exception raising.
>
> (the "problem mentioned here" is already avoided in the patch, but the
> detection of other cycles is deferred to exception reporting for the
> reason given above)

I meant only that trivial cycles should be detected.  However, I
hadn't read your patch, so I didn't realize you already knew of a way
to create a non-trivial cycle.

This has placed a niggling doubt in my mind about chaining the
exceptions, rather than the tracebacks.  Hrm.

>> * PyErr_Display is used by PyErr_Print, and it must end up with no
>> active exception.  Additionally, third party code may depend on this
>> semantic.  Maybe PyErr_DisplayEx?
>
> I was not proposing to change the exception swallowing semantics, just
> to add a return value indicating if any errors had occurred while
> displaying the exception.

Ahh, harmless then, but to what benefit?  Wouldn't the traceback
module be better suited to any possible error reporting?
msg68575 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-22 19:04
Le dimanche 22 juin 2008 à 17:17 +0000, Adam Olsen a écrit :
> I meant only that trivial cycles should be detected.  However, I
> hadn't read your patch, so I didn't realize you already knew of a way
> to create a non-trivial cycle.
> 
> This has placed a niggling doubt in my mind about chaining the
> exceptions, rather than the tracebacks.  Hrm.

Chaining the tracebacks rather than the exceptions loses important
information: what is the nature of the exception which is the cause or
context of the current exception?

It is improbable to create such a cycle involuntarily, it means you
raise an old exception in replacement of a newer one caused by the
older, which I think is quite contorted. It is also quite easy to avoid
creating the cycle, simply by re-raising outside of any except handler.

> Ahh, harmless then, but to what benefit?

I don't know, I've never used that API, I just proposed returning that
error code since it is computed inside the function anyway.
But let's drop that proposal. Same for PyErr_DisplaySingle if nobody
cares.

> Wouldn't the traceback
> module be better suited to any possible error reporting?

I suppose built-in reporting is needed when reporting a grave error such
as a MemoryError (C code can avoid doing any memory allocations), or an
error that occurred during startup before any pure Python modules could
be initialized (how would you report a syntax error in the traceback
module itself?).

But application code should use the traceback module rather than try to
access the C API.
msg68576 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-22 19:23
On Sun, Jun 22, 2008 at 1:04 PM, Antoine Pitrou <report@bugs.python.org> wrote:
>
> Antoine Pitrou <pitrou@free.fr> added the comment:
>
> Le dimanche 22 juin 2008 à 17:17 +0000, Adam Olsen a écrit :
>> I meant only that trivial cycles should be detected.  However, I
>> hadn't read your patch, so I didn't realize you already knew of a way
>> to create a non-trivial cycle.
>>
>> This has placed a niggling doubt in my mind about chaining the
>> exceptions, rather than the tracebacks.  Hrm.
>
> Chaining the tracebacks rather than the exceptions loses important
> information: what is the nature of the exception which is the cause or
> context of the current exception?

I assumed each leg of the traceback would reference the relevant exception.

Although.. this is effectively the same as creating a new exception
instance when reraised, rather than modifying the old one.  Reusing
the old is done for performance I believe.

> It is improbable to create such a cycle involuntarily, it means you
> raise an old exception in replacement of a newer one caused by the
> older, which I think is quite contorted. It is also quite easy to avoid
> creating the cycle, simply by re-raising outside of any except handler.

I'm not convinced.

    try:
        ...  # Lookup
    except A as a:  # Lookup failed
        try:
            ...  # Fallback
        except B as b:  # Fallback failed
            raise a  # The original exception is of the type we want

For this behaviour, this is the most natural way to write it.
Conceptually, there shouldn't be a cycle - the traceback should be the
lookup, then the fallback, then whatever code is about this - exactly
the order the code executed in.
msg68578 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-22 19:48
Le dimanche 22 juin 2008 à 19:23 +0000, Adam Olsen a écrit :
> For this behaviour, this is the most natural way to write it.
> Conceptually, there shouldn't be a cycle

I agree your example is not far-fetched. How about avoiding cycles for
implicit chaining, and letting users shoot themselves in the foot with
explicit recursive chaining if they want? Detection would be cheap
enough, just a simple loop without any memory allocation.

> the traceback should be the
> lookup, then the fallback, then whatever code is about this - exactly
> the order the code executed in.

It would be the reverse: first the fallback, then the re-raised
exception. The last caught exception is always reported last, because
it's supposed to be the "main" or "highest-level" one.
msg68579 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-22 19:57
On Sun, Jun 22, 2008 at 1:48 PM, Antoine Pitrou <report@bugs.python.org> wrote:
>
> Antoine Pitrou <pitrou@free.fr> added the comment:
>
> Le dimanche 22 juin 2008 à 19:23 +0000, Adam Olsen a écrit :
>> For this behaviour, this is the most natural way to write it.
>> Conceptually, there shouldn't be a cycle
>
> I agree your example is not far-fetched. How about avoiding cycles for
> implicit chaining, and letting users shoot themselves in the foot with
> explicit recursive chaining if they want? Detection would be cheap
> enough, just a simple loop without any memory allocation.

That's still O(n).  I'm not so easily convinced it's cheap enough.

And for that matter, I'm not convinced it's correct.  The inner
exception's context becomes clobbered when we modify the outer
exception's traceback.  The inner's context should reference the
traceback as it was at that point.

This would all be a lot easier if reraising always created a new
exception.  Can you think of a way to skip that only when we can be
sure its safe?  Maybe as simple as counting the references to it?
msg68580 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-22 20:20
Le dimanche 22 juin 2008 à 19:57 +0000, Adam Olsen a écrit :
> That's still O(n).  I'm not so easily convinced it's cheap enough.

O(n) when n will almost never be greater than 5 (and very often equal to
1 or 2), and when the unit is the cost of a pointer dereference plus the
cost of a pointer comparison, still sounds cheap. We could bench it
anyway.

> And for that matter, I'm not convinced it's correct.  The inner
> exception's context becomes clobbered when we modify the outer
> exception's traceback.  The inner's context should reference the
> traceback as it was at that point.

Yes, I've just thought about that, it's a bit annoying... We have to
decide what is more annoying: that, or a reference cycle that can delay
deallocation of stuff attached to an exception (including local
variables attached to the tracebacks)?

(just a small note: it's exception objects that are chained, not
tracebacks... we never modify tracebacks at any point)

> This would all be a lot easier if reraising always created a new
> exception.

How do you duplicate an instance of an user-defined exception? Using an
equivalent of copy.deepcopy()? It will probably end up much more
expensive than the above-mentioned O(n) search.

> Can you think of a way to skip that only when we can be
> sure its safe?  Maybe as simple as counting the references to it?

I don't think so, the exception can be referenced in an unknown number
of local variables (themselves potentially referenced by tracebacks).
msg68581 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-22 20:40
On Sun, Jun 22, 2008 at 2:20 PM, Antoine Pitrou <report@bugs.python.org> wrote:
>
> Antoine Pitrou <pitrou@free.fr> added the comment:
>
> Le dimanche 22 juin 2008 à 19:57 +0000, Adam Olsen a écrit :
>> That's still O(n).  I'm not so easily convinced it's cheap enough.
>
> O(n) when n will almost never be greater than 5 (and very often equal to
> 1 or 2), and when the unit is the cost of a pointer dereference plus the
> cost of a pointer comparison, still sounds cheap. We could bench it
> anyway.

Indeed.

>> And for that matter, I'm not convinced it's correct.  The inner
>> exception's context becomes clobbered when we modify the outer
>> exception's traceback.  The inner's context should reference the
>> traceback as it was at that point.
>
> Yes, I've just thought about that, it's a bit annoying... We have to
> decide what is more annoying: that, or a reference cycle that can delay
> deallocation of stuff attached to an exception (including local
> variables attached to the tracebacks)?

The cycle is only created by broken behaviour.  The more I think about
it, the more I want to fix it (by not reusing the exception).

>> This would all be a lot easier if reraising always created a new
>> exception.
>
> How do you duplicate an instance of an user-defined exception? Using an
> equivalent of copy.deepcopy()? It will probably end up much more
> expensive than the above-mentioned O(n) search.

Passing in e.args is probably sufficient.  All this would need to be
discussed on python-dev (or python-3000?) though.

>> Can you think of a way to skip that only when we can be
>> sure its safe?  Maybe as simple as counting the references to it?
>
> I don't think so, the exception can be referenced in an unknown number
> of local variables (themselves potentially referenced by tracebacks).

Can be, or will be?  Only the most common behaviour needs to be optimized.
msg68582 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-22 20:56
Le dimanche 22 juin 2008 à 20:40 +0000, Adam Olsen a écrit :
> > How do you duplicate an instance of an user-defined exception? Using
> an
> > equivalent of copy.deepcopy()? It will probably end up much more
> > expensive than the above-mentioned O(n) search.
> 
> Passing in e.args is probably sufficient.

I think it's very optimistic :-) Some exception objects can hold dynamic
state which is simply not stored in the "args" tuple. See Twisted's
Failure objects for an extreme example:
http://twistedmatrix.com/trac/browser/trunk/twisted/python/failure.py

(yes, it is used an an exception: see "raise self" in the trap() method)

> Can be, or will be?  Only the most common behaviour needs to be optimized.

Well, the "most common behaviour" is a very short context chain, which
is already optimized by my reference-avoidance proposal...
msg68611 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-23 06:16
On Sun, Jun 22, 2008 at 2:56 PM, Antoine Pitrou <report@bugs.python.org> wrote:
> Le dimanche 22 juin 2008 à 20:40 +0000, Adam Olsen a écrit :
>> Passing in e.args is probably sufficient.
>
> I think it's very optimistic :-) Some exception objects can hold dynamic
> state which is simply not stored in the "args" tuple. See Twisted's
> Failure objects for an extreme example:
> http://twistedmatrix.com/trac/browser/trunk/twisted/python/failure.py
>
> (yes, it is used an an exception: see "raise self" in the trap() method)

Failure doesn't have an args tuple and doesn't subclass Exception (or
BaseException) - it already needs modification in 3.0.  It's heaped
full of complexity and implementation details.  I wouldn't be
surprised if your changes break it in subtle ways too.

In short, if forcing Failure to be rewritten is the only consequence
of using .args, it's an acceptable tradeoff of not corrupting
exception contexts.
msg68612 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-23 07:08
Le lundi 23 juin 2008 à 06:16 +0000, Adam Olsen a écrit :
> Failure doesn't have an args tuple and doesn't subclass Exception (or
> BaseException) - it already needs modification in 3.0.  It's heaped
> full of complexity and implementation details.  I wouldn't be
> surprised if your changes break it in subtle ways too.

Failure was only an example, I'm sure there are lots of other ones. The
point is that the assertion that user-defined exceptions can be safely
cloned by cloning their args is wrong, unless you want to put very
strong restrictions on how user-defined exceptions can behave.
msg68845 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-06-27 19:13
Here is the final patch. Features:
- cleanup of internal APIs
- standardize traceback indentation (source lines are prefixed with 4
spaces)
- break cycles along the context chain (a synthetic benchmark with a
6-level deep context chain shows a very moderate slowdown of 10-15%)
msg69072 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-07-01 20:38
Potential reviewers, let's make life easier for you:
http://codereview.appspot.com/2448
msg69547 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-07-11 12:12
I think there is a problem when the source file cannot be opened (a .pyc
without its .py): the four spaces are printed, but not the line, and the
following level is not properly indented.

See issue3342 for a competing patch that corrects the indentation problem.
msg69559 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-07-11 18:51
Le vendredi 11 juillet 2008 à 12:12 +0000, Amaury Forgeot d'Arc a
écrit :
> I think there is a problem when the source file cannot be opened (a .pyc
> without its .py): the four spaces are printed, but not the line, and the
> following level is not properly indented.
> 
> See issue3342 for a competing patch that corrects the indentation problem.

Great! Do you plan to commit it soon or should I include it into my
patch?

(and do you have other observations on the present patch?)
msg69568 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-07-11 22:18
I committed r64881, which invalidates your patch a bit :-|

BTW, I don't like your comment "Can't be bothered to check all those
PyFile_WriteString() calls". 
In general, it is not a good idea to execute python code with an
exception set, this leads to subtle problems, and some "XXX undetected
error" prints in debug mode. Chaining several calls to PyDict_SetItem
for example is usually not a problem, but PyFile_WriteString does
execute python code in python3.0.
msg69572 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-07-11 23:20
Le vendredi 11 juillet 2008 à 22:18 +0000, Amaury Forgeot d'Arc a
écrit :
> I committed r64881, which invalidates your patch a bit :-|

Apparently you committed in trunk rather than py3k? Could you svnmerge
into py3k as well? Then it should be quite easy to rework my patch
around it.

> BTW, I don't like your comment "Can't be bothered to check all those
> PyFile_WriteString() calls". 

It's not my comment, it was already there. I agree it doesn't sound very
meticulous. :-)

> In general, it is not a good idea to execute python code with an
> exception set, this leads to subtle problems, and some "XXX undetected
> error" prints in debug mode.
> Chaining several calls to PyDict_SetItem
> for example is usually not a problem, but PyFile_WriteString does
> execute python code in python3.0.

Well, as said above, I just kept the original method of doing things...
If you think of a simple solution to make things better (and a test case
to validate it), I'm open to integrating it in the patch.
msg69622 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-07-13 20:45
Here is a new patch incorporating Amaury's better indentation fix for
tracebacks. It should be ready for consumption (all the tests pass).

And for the code review junkies: http://codereview.appspot.com/2448
msg69681 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-07-15 13:50
Antonine, do you have a patch address the review comments?
msg69684 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-07-15 14:24
> Antonine, do you have a patch address the review comments?

Yes, although I've forgotten to upload it here - you will find it at
http://codereview.appspot.com/2448

PS: it is Antoine not Antonine :)
msg69690 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-07-15 15:32
Sorry, Antoine! The patch is in with r64965.
History
Date User Action Args
2008-07-15 15:32:27benjamin.petersonsetstatus: open -> closed
resolution: fixed
messages: + msg69690
2008-07-15 14:24:13pitrousetmessages: + msg69684
2008-07-15 13:50:10benjamin.petersonsetassignee: benjamin.peterson
messages: + msg69681
2008-07-13 20:45:46pitrousetfiles: + exc_reporting2.patch
messages: + msg69622
2008-07-11 23:20:31pitrousetmessages: + msg69572
2008-07-11 22:18:22amaury.forgeotdarcsetmessages: + msg69568
2008-07-11 18:51:46pitrousetmessages: + msg69559
2008-07-11 12:12:56amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg69547
2008-07-01 20:38:27pitrousetmessages: + msg69072
2008-06-27 19:13:24pitrousetfiles: + exc_reporting.patch
messages: + msg68845
2008-06-23 07:08:33pitrousetmessages: + msg68612
2008-06-23 06:16:04Rhamphoryncussetmessages: + msg68611
2008-06-22 20:56:07pitrousetmessages: + msg68582
2008-06-22 20:40:19Rhamphoryncussetmessages: + msg68581
2008-06-22 20:20:03pitrousetmessages: + msg68580
2008-06-22 19:57:52Rhamphoryncussetmessages: + msg68579
2008-06-22 19:48:54pitrousetmessages: + msg68578
2008-06-22 19:23:22Rhamphoryncussetmessages: + msg68576
2008-06-22 19:04:28pitrousetmessages: + msg68575
2008-06-22 17:17:35Rhamphoryncussetmessages: + msg68569
2008-06-22 14:07:13pitrousetmessages: + msg68565
2008-06-22 07:04:57Rhamphoryncussetnosy: + Rhamphoryncus
messages: + msg68559
2008-06-19 22:35:22pitrousetmessages: + msg68427
2008-06-18 22:24:42pitrousetfiles: + exc_reporting.patch
keywords: + patch
messages: + msg68380
2008-06-18 21:14:30pitrousetmessages: + msg68375
2008-06-18 18:43:10pitrousetmessages: + msg68370
2008-06-18 18:42:13pitrousetnosy: + gvanrossum
2008-06-14 20:47:00pitrousetmessages: + msg68217
2008-06-14 20:39:11pitrousetnosy: + pitrou
2008-06-14 20:32:06benjamin.petersonsetversions: + Python 3.0, - Python 2.6
2008-06-14 20:30:45benjamin.petersoncreate