classification
Title: generator bug with exception: tstate->exc_value is not cleared after an except block
Type: Stage: resolved
Components: Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, pitrou, python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-01-30 14:08 by vstinner, last changed 2015-03-18 21:29 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
excinfo_bug2.py vstinner, 2015-01-30 14:08
excinfo_bug6.py vstinner, 2015-01-30 15:57
gen_exc_value.patch vstinner, 2015-01-31 00:48 review
gen_exc_value_py27.patch vstinner, 2015-01-31 01:24 review
gen_exc_state_restore.patch pitrou, 2015-01-31 03:32 review
gen_exc_value_py27.patch vstinner, 2015-01-31 10:15 review
exctests.patch pitrou, 2015-01-31 15:06 review
Messages (23)
msg235033 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-30 14:08
Since my changeset a5efd5021ca1, the Python test suite starts to fail randomly. Running test_asyncio modifies sys.exc_info(): it is not (None, None, None) after the execution of test_asyncio. The problem comes from test_cancel_make_subprocess_transport_exec() of Lib/test/test_asyncio/test_subprocess.py.

I tried to write a simple script which does not depend on Python to reproduce the issue. See attached excinfo_bug2.py script.

Output:
---
exc_info after except (<class '__main__.MyException'>, MyException(), <traceback object at 0x7f09201ad7c8>)
exc_info at exit (<class '__main__.MyException'>, MyException(), <traceback object at 0x7f09201abd08>)
---

exc_info is supposed to be (None, None, None), at least at exit.

I will try to write an even simpler script to identify the bug.
msg235037 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-30 15:57
I simplified the script even more: 287 lines (6 functions/generators, 7 classes/exceptions) => 28 lines (1 generator)!
msg235067 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-31 00:22
Last major change related to generators in Python/ceval.c:
---
changeset:   47594:212a1fee6bf9
parent:      47585:b0ef00187a7e
user:        Benjamin Peterson <benjamin@python.org>
date:        Wed Jun 11 15:59:43 2008 +0000
files:       Doc/library/dis.rst Doc/library/inspect.rst Doc/library/sys.rst Doc/reference/datamodel.rst Include/frameobject.h Include/opcode.h Lib/doctest.py Li
description:
#3021: Antoine Pitrou's Lexical exception handlers
---

This change introduced SWAP_EXC_STATE() and SAVE_EXC_STATE().
msg235069 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-31 00:48
Attached gen_exc_value.patch changes how generators handle the currently handled exception (tstate->exc_value). The patch probably lacks tests to test the exact behaviour of sys.exc_info(). The 3 examples below can be used to write such tests. But before investing time on an implemenation, I would like to ensure that I correctly understood the bug and discuss how it should be fixed.

Currently, a generator inherits the currently handled exception from the "caller" (function calling next(), gen.send() or gen.throw()). With the patch, the generator and its caller don't share the exception anymore. The generator still remembers the current exception when it is interrupted by yield.

It's still possible to pass the exception from the caller to the generator using the throw() method of the generator. My patch doesn't change the behaviour of throw(): see the example 3 below.

The patch also fixes the initial issue: "./python -m test test_asyncio test_functools" doesn't fail anymore.

Python 2.7 is also affected by the bug. I don't know yet if it's safe to change the behaviour of currently handled exception in Python 2 generators. It may break backward compatibility. We should probably check applications which heavily depends on generators. For example, the Trollius and Twisted projects use generators for coroutines in asynchronous programming.

The backward compatibility also matters in Python 3.4, so same question: should we change the behaviour of Python 3.4? Can it break applications?

In Python 3, the currently handled exception is more important than in Python 2, because it is used to automatically fill the __context__ attribute of raised exceptions.

I didn't test the behaviour of yield-from yet, I don't know how my patch changes its behaviour.


Example 1:
---
import sys

def gen():
    print(sys.exc_info())
    yield

g = gen()
try:
    raise ValueError
except Exception:
    next(g)
---

Original output:
---
(<class 'ValueError'>, ValueError(), <traceback object at 0x7f22a1ab52c8>)
---

With the patch:
---
(None, None, None)
---


Example 2:
---
import sys

def gen():
    try:
        yield
        raise TypeError()
    except:
        print(sys.exc_info())
    print(sys.exc_info())
    yield

g = gen()
next(g)
try:
    raise ValueError
except Exception:
    next(g)
---

Original output:
---
(<class 'TypeError'>, TypeError(), <traceback object at 0x7fad239a22c8>)
(<class 'ValueError'>, ValueError(), <traceback object at 0x7fad239a2288>)
---

With the patch:
---
(<class 'TypeError'>, TypeError(), <traceback object at 0x7f278b174988>)
(None, None, None)
---


Example 3:
---
import sys

def gen():
    try:
        try:
            yield
        except:
            print(sys.exc_info())
            raise TypeError()
    except Exception as exc:
        print("TypeError context:", repr(exc.__context__))
    yield

g = gen()
next(g)
try:
    raise ValueError
except Exception as exc:
    g.throw(exc)
---

Original output:
---
(<class 'ValueError'>, ValueError(), <traceback object at 0x7f233f05e388>)
TypeError context: ValueError()
(<class 'ValueError'>, ValueError(), <traceback object at 0x7f233f05e348>)
---

With the patch (unchanged):
---
(<class 'ValueError'>, ValueError(), <traceback object at 0x7fdf356fead8>)
TypeError context: ValueError()
(None, None, None)
---
msg235070 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-31 00:53
See also:

* PEP 3134: Exception Chaining and Embedded Tracebacks (Python 3.0)
* Issue #3021: Lexical exception handlers (Python 3.0) -- thread: https://mail.python.org/pipermail/python-3000/2008-May/013740.html
* PEP 380: Syntax for Delegating to a Subgenerator (Python 3.3)
msg235072 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-31 01:10
Oh, by the way: keeping the exception after the except block is also a tricky reference leak. In Python 3, since exceptions store their traceback, this issue may keep a lot of objects alive too long, whereas they are expected to be destroyed much earlier.

When I started to investigate this issue, it took me 2 hours to begin to understand why so many objects were kept alive. It looks like a reference cycle, a reference leak, or other kind of complex memory leak. Clearing manually local variables (ex: "self = None" in methods) is not enough.

Python 2 has a sys.exc_clear() method which can be used to workaround this issue. It cannot be used in Python 3 since the function was removed in Python 3.
msg235073 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-31 01:16
> Currently, a generator inherits the currently handled exception from
> the "caller" 

This is expected, since this is how normal functions behave.
msg235074 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-31 01:24
Attached gen_exc_value_py27.patch: Patch for Python 2.7. No unit test yet.

The full test suite of trollius pass on the patched Python 2.7 and on the patched Python 3.5. The full test suite of asyncio also pass on the patched Python 3.5.
msg235076 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-31 01:38
> > Currently, a generator inherits the currently handled exception from
> > the "caller"
>
> This is expected, since this is how normal functions behave.

Do you see how to fix the issue without changing the behaviour?
msg235077 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-31 03:32
Attached patch fixes the test script and doesn't break any test.
msg235078 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-31 03:36
Note the patch also fixes the reference leak in test_asyncio.
msg235097 - (view) Author: Roundup Robot (python-dev) Date: 2015-01-31 10:09
New changeset 4555da8c3091 by Victor Stinner in branch '3.4':
Issue #23353: Fix the exception handling of generators in PyEval_EvalFrameEx().
https://hg.python.org/cpython/rev/4555da8c3091
msg235099 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-31 10:15
I agree that it's better to not change the behaviour of generators, backward compatibility matters :-)

I wrote tests using my examples and I combined them with  gen_exc_state_restore.patch. I commited the changeset in Python 3.4 and 3.5.


Backporting the fix to Python 2.7 looks more complex because the EXCEPT_HANDLE try block type and the POP_EXCEPT instruction are new in Python 3.0: introduced by 212a1fee6bf9 from the issue #3021.

What do you think? Is it worth to fix this issue in Python 2.7?

I plan to workaround this bug in Tulip to support Python 3.3. I will also workaround it in Trollius to support Python 2.6 and newer. So for me, it's ok to live with this known bug.

It's just yet another generator bug. asyncio/trollius already work around a yield-from bug (issue #21209) ;-)

> Note the patch also fixes the reference leak in test_asyncio.

Yes, as I explained in msg235072, this bug caused strange "memory leaks".
msg235104 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-31 12:24
I think your patch for 2.7 is wrong as was your patch for 3.x. You shouldn't change the behaviour of sys.exc_info() in the nominal case.
msg235110 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-31 14:47
It would have been nice to wait for a review. Generator tests are already in test_exceptions.py.
msg235112 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-31 15:06
Additional simple tests for test_exceptions.py.
msg235115 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-01-31 16:15
If you need non-normalized exception for testing, a NameError generated by interpreter is not normalized.
msg235116 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-31 16:27
> It would have been nice to wait for a review. Generator tests are already in test_exceptions.py.

Sorry, I wanted to quickly push your fix to fix buildbots. I dislike being the responsible of turning all buildbots to red...

Before working on this issue, I didn't know test_generators. Well, I didn't know test_exceptions neither :-) test_exceptions.py sounds like a better name for checks on the currently handled exception :-)

I saw that test_generators.py is mostly written with doctests. At the beginning, doctests were shiny and fun. Now I consider that it's worse than classic unit tests and I plan to rewrite doctests to unittest.TestCase classes. I will open a new issue for that.

> I think your patch for 2.7 is wrong as was your patch for 3.x. You shouldn't change the behaviour of sys.exc_info() in the nominal case.

I now agree that gen_exc_value.patch was wrong. gen_exc_value_py27.patch was just a backport of my patch to Python 2.7.

(Oh I see that I uploaded gen_exc_value_py27.patch twice, it's a mistake.)

In my previous message, I asked myself if it would be possible to backport your patch (gen_exc_state_restore.patch) to Python 2.7.
msg235117 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-31 16:28
By the way: buildbots are green again, cool.
msg235277 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-02 17:38
New changeset 2cd6621a9fbc by Victor Stinner in branch '3.4':
Issue #23353, asyncio: Workaround CPython bug #23353
https://hg.python.org/cpython/rev/2cd6621a9fbc
msg238403 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-18 10:48
@Antoine: exctests.patch looks good to me, can you commit it?
msg238470 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-18 21:25
New changeset ac43268da908 by Antoine Pitrou in branch '3.4':
Issue #23353: improve exceptions tests for generators
https://hg.python.org/cpython/rev/ac43268da908

New changeset b29342f53174 by Antoine Pitrou in branch 'default':
Issue #23353: improve exceptions tests for generators
https://hg.python.org/cpython/rev/b29342f53174
msg238472 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-18 21:29
Thanks Antoine!
History
Date User Action Args
2015-03-18 21:29:02vstinnersetmessages: + msg238472
2015-03-18 21:25:33pitrousetstatus: open -> closed
resolution: fixed
stage: resolved
2015-03-18 21:25:13python-devsetmessages: + msg238470
2015-03-18 10:48:31vstinnersetmessages: + msg238403
2015-02-08 07:31:04Arfreversetnosy: + Arfrever
2015-02-02 17:38:37python-devsetmessages: + msg235277
2015-01-31 16:28:49vstinnersetmessages: + msg235117
2015-01-31 16:27:14vstinnersetmessages: + msg235116
2015-01-31 16:15:18serhiy.storchakasetmessages: + msg235115
2015-01-31 15:06:54pitrousetfiles: + exctests.patch

messages: + msg235112
2015-01-31 14:47:47pitrousetmessages: + msg235110
2015-01-31 12:24:57pitrousetmessages: + msg235104
2015-01-31 10:15:40vstinnersetfiles: + gen_exc_value_py27.patch

messages: + msg235099
2015-01-31 10:09:07python-devsetnosy: + python-dev
messages: + msg235097
2015-01-31 03:36:30pitrousetmessages: + msg235078
2015-01-31 03:32:48pitrousetfiles: + gen_exc_state_restore.patch

messages: + msg235077
2015-01-31 01:38:15vstinnersetmessages: + msg235076
2015-01-31 01:24:39vstinnersetfiles: + gen_exc_value_py27.patch

messages: + msg235074
2015-01-31 01:16:57pitrousetmessages: + msg235073
2015-01-31 01:10:21vstinnersetmessages: + msg235072
2015-01-31 00:53:11vstinnersetmessages: + msg235070
2015-01-31 00:48:35vstinnersetnosy: + pitrou, serhiy.storchaka
2015-01-31 00:48:20vstinnersetfiles: + gen_exc_value.patch
keywords: + patch
messages: + msg235069

versions: + Python 2.7
2015-01-31 00:22:21vstinnersetmessages: + msg235067
title: gnerator bug with exception: tstate->exc_value is not cleared after an except block -> generator bug with exception: tstate->exc_value is not cleared after an except block
2015-01-30 15:57:44vstinnersetfiles: + excinfo_bug6.py

messages: + msg235037
2015-01-30 14:08:56vstinnercreate