classification
Title: generator.throw() ignores __traceback__ of exception
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Trundle, ezio.melotti, ncoghlan, petri.lehtinen, pitrou, python-dev, yak
Priority: normal Keywords: patch

Created on 2011-10-15 23:34 by pitrou, last changed 2011-10-28 05:23 by petri.lehtinen. This issue is now closed.

Files
File name Uploaded Description Edit
issue13188.patch petri.lehtinen, 2011-10-18 13:17 review
issue13188_v2.patch petri.lehtinen, 2011-10-18 13:46 review
Messages (14)
msg145609 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-15 23:34
In the following code, the original traceback attached to the exception thrown into the generator is ignored:

def gen():
    try:
        yield
    except:
        raise

g = gen()
try:
    1/0
except ZeroDivisionError as v:
    g.throw(v)


But if you replace the last line with:

    g.throw(type(v), v, v.__traceback__)

then the original traceback gets appended.
g.throw() should have fetched the __traceback__ attribute by itself.
msg145806 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-10-18 13:17
Attached a patch that fixes this. The test case is a bit ugly, as it only checks that the traceback's depth is correct.
msg145808 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-18 13:28
Thank you! There is a memory leak somewhere:

$ ./python -m test -R 3:2 test_generators
[1/1] test_generators
beginning 5 repetitions
12345
.....
test_generators leaked [1945, 1945] references, sum=3890
1 test failed:
    test_generators


Also, since the patch is so short, a stylistic nit:

+            if(tb == NULL) {

There should be a space after the "if".
(see PEP 7: http://www.python.org/dev/peps/pep-0007/)
msg145809 - (view) Author: Andreas Stührk (Trundle) Date: 2011-10-18 13:39
It leaks because `PyException_GetTraceback()` already returns a new reference, hence the "Py_XINCREF(tb)" is wrong.
msg145811 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-10-18 13:46
Attached an updated patch.

I incref'd the return value of PyErr_GetTraceback() because PyErr_Restore() steals the reference. The documentation of PyErr_GetTraceback() doesn't tell whether it returns a new or borrowed reference, but apparently a new reference then?
msg145813 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-18 14:44
Actually, it is documented:
“Return the traceback associated with the exception as a new reference (...)”
http://docs.python.org/dev/c-api/exceptions.html#PyException_GetTraceback

Thanks for the patch, will apply!
msg145814 - (view) Author: Roundup Robot (python-dev) Date: 2011-10-18 14:47
New changeset dcf5cc88d5c9 by Antoine Pitrou in branch '3.2':
Issue #13188: When called without an explicit traceback argument,
http://hg.python.org/cpython/rev/dcf5cc88d5c9

New changeset f4e3db1194e4 by Antoine Pitrou in branch 'default':
Issue #13188: When called without an explicit traceback argument,
http://hg.python.org/cpython/rev/f4e3db1194e4
msg145843 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-10-18 17:39
Antoine Pitrou wrote:
> Actually, it is documented:
> “Return the traceback associated with the exception as a new reference (...)”

Ah, you're right. It just doesn't have the green "Return value: New
reference" note.

Thanks for committing!
msg145845 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-10-18 17:50
BTW, shouldn't this be applied to 2.7 too?
msg146394 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-10-25 19:20
The same issue exists on 2.7, working on a patch.
msg146447 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-10-26 18:11
It seems this cannot be achieved the same way in 2.7 as the traceback is not directly associated with the exception. However, if we're currently in an exception handler and sys.exc_info() corresponds to the exception passed to generator.throw(), we could use the current traceback. Would this approach be too complicated and not worth it?
msg146486 - (view) Author: Arkadiusz Wahlig (yak) Date: 2011-10-27 13:54
I don't think this should be applied to 2.7.

In 2.x, the full exception info consists of the (type, value, traceback)-trio. Programmer is expected to pass this around to retain full exception info. Re-raising just the value (exception instance) using "raise value" starts a fresh traceback. Currently "raise" and gen.throw() behave the same in that regard and there's no reason to change that.

Python 3 reduces the exception info to one object by adding __traceback__ to the exception instance so it's reasonable to expect that "raise" and gen.throw() will be able to extract the full info from just the instance.

"raise" does that today, gen.throw() didn't, hence this bug report.
msg146525 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-27 22:16
I agree with Arkadiusz, this doesn't seem to match Python 2's exception semantics, where you always specify the traceback explicitly if you want to.
msg146535 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-10-28 05:23
Ok, I think we have reached a consensus. Closing.
History
Date User Action Args
2011-10-28 05:23:35petri.lehtinensetstatus: open -> closed

messages: + msg146535
versions: - Python 2.7
2011-10-27 22:16:08pitrousetmessages: + msg146525
2011-10-27 13:54:43yaksetnosy: + yak
messages: + msg146486
2011-10-26 18:11:09petri.lehtinensetmessages: + msg146447
2011-10-25 19:20:30petri.lehtinensetstatus: closed -> open

messages: + msg146394
versions: + Python 2.7
2011-10-18 17:50:34petri.lehtinensetmessages: + msg145845
2011-10-18 17:39:26petri.lehtinensetmessages: + msg145843
2011-10-18 14:47:33pitrousetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2011-10-18 14:47:03python-devsetnosy: + python-dev
messages: + msg145814
2011-10-18 14:44:17pitrousetmessages: + msg145813
2011-10-18 13:46:34petri.lehtinensetfiles: + issue13188_v2.patch

messages: + msg145811
2011-10-18 13:39:25Trundlesetnosy: + Trundle
messages: + msg145809
2011-10-18 13:28:13pitrousetmessages: + msg145808
2011-10-18 13:17:36petri.lehtinensetfiles: + issue13188.patch

nosy: + ezio.melotti, petri.lehtinen
messages: + msg145806

keywords: + patch
stage: patch review
2011-10-15 23:34:29pitroucreate