Generator/coroutine 'throw' discards exc_info state, which is bad

[njsmith]

BPO	29587
Nosy	@gvanrossum, @vstinner, @ned-deily, @njsmith, @cjerdonek, @hynek, @vadmium, @1st1
PRs	bpo-29587: Enable implicit exception chaining with gen.throw() #19811 Revert "bpo-29587: Enable implicit exception chaining with gen.throw()" #19821 bpo-29587: Enable implicit exception chaining with gen.throw() #19823 bpo-29587: Add another test for the gen.throw() exception chain fix #19859 bpo-29587: Remove the exc_value NULL check in _gen_throw() #19877 bpo-29587: PyErr_SetObject() rejects exception=None #19902 bpo-29587: Enable exception chaining for gen.throw() with "yield from" #19858

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-05-02.01:26:31.061>
created_at = <Date 2017-02-17.11:01:49.760>
labels = ['type-bug', '3.9']
title = "Generator/coroutine 'throw' discards exc_info state, which is bad"
updated_at = <Date 2020-05-17.04:14:59.324>
user = 'https://github.com/njsmith'

bugs.python.org fields:

activity = <Date 2020-05-17.04:14:59.324>
actor = 'chris.jerdonek'
assignee = 'none'
closed = True
closed_date = <Date 2020-05-02.01:26:31.061>
closer = 'chris.jerdonek'
components = []
creation = <Date 2017-02-17.11:01:49.760>
creator = 'njs'
dependencies = []
files = []
hgrepos = []
issue_num = 29587
keywords = ['patch']
message_count = 30.0
messages = ['287987', '287992', '306100', '306158', '367747', '367773', '367774', '367776', '367783', '367784', '367786', '367787', '367788', '367789', '367792', '367827', '367833', '367905', '367907', '367908', '367924', '367926', '367954', '367956', '368013', '368053', '368164', '368617', '368806', '369093']
nosy_count = 8.0
nosy_names = ['gvanrossum', 'vstinner', 'ned.deily', 'njs', 'chris.jerdonek', 'hynek', 'martin.panter', 'yselivanov']
pr_nums = ['19811', '19821', '19823', '19859', '19877', '19902', '19858']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29587'
versions = ['Python 3.9']

[njsmith]

Example 1:

-----------

def f():
    try:
        raise KeyError
    except Exception:
        yield

gen = f()
gen.send(None)
gen.throw(ValueError)

---

Output:
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 5, in f
ValueError

Expected output:

Something involving the string "During handling of the above exception, another exception occurred", and a traceback for the original KeyError.

Example 2:

-----------

import sys

def f():
    try:
        raise KeyError
    except Exception:
        print(sys.exc_info())
        try:
            yield
        except Exception:
            pass
        print(sys.exc_info())
        raise

gen = f()
gen.send(None)
gen.throw(ValueError)

---

Output:

(<class 'KeyError'>, KeyError(), <traceback object at 0x7f67ce3c3f88>)
(None, None, None)
Traceback (most recent call last):
  File "/tmp/foo.py", line 17, in <module>
    gen.throw(ValueError)
  File "/tmp/foo.py", line 13, in f
    raise
RuntimeError: No active exception to reraise

Expected output: certainly not that :-)

This seems to happen because normally, generators save the current exc_info when yielding, and then restore it when re-entered. But, if we re-enter through 'throw' (throwflag is true), this is disabled:

cpython/Python/ceval.c

Line 1027 in b2ee40e

if (!throwflag && f->f_exc_type != NULL && f->f_exc_type != Py_None) {

This check seems to have been added in ae5f2f4 as a fix for:

https://bugs.python.org/issue7173

which had to do with a nasty situation involving a generator object that was part of a reference cycle: the gc sometimes would free the objects stored in the generator's saved exc_info, and then try to clean up the generator by throwing in a GeneratorExit.

AFAICT this situation shouldn't be possible anymore thanks to PEP-442, which makes it so that finalizers are run before any part of the cycle is freed. And in any case it certainly doesn't justify breaking code like the examples above.

(Note: the examples use generators for simplicity, but of course the way I noticed this was that I had some async/await code where exceptions were mysteriously disappearing instead of showing up in __context__ and couldn't figure out why. It's likely that more people will run into this in the future as async/await becomes more widely used. As a workaround for now I'll probably modify my coroutine runner so that it never uses 'throw'.)

[vadmium]

The second example seems like the original complaint in bpo-25612. That spawned a separate bug related to the first situation, which was later closed: bpo-25683.

[gvanrossum]

Yury do you agree this is a bug? Do you have any ideas on how to fix it?

[1st1]

Guido,

The second example ("No active exception to reraise") was an actual long open bug. It was recently fixed by Mark Shannon in [1].

I think we should be able to fix the first case (missing __context__) although it's not critical. I'll look into it after the context PEP and few other open asyncio issues.

[1] #1773

[cjerdonek]

I made a naive attempt at addressing this issue here:
#19811

The code has diverged significantly since Nathaniel first linked to a specific line above. However, what I came up with is simple, and seems to work. But I could very well be missing some things.

It would be great if this could be fixed in some way.

[gvanrossum]

New changeset 2514a63 by Chris Jerdonek in branch 'master':
bpo-29587: Enable implicit exception chaining with gen.throw() (GH-19811)
2514a63

[gvanrossum]

Example 1 is now fixed too by Chris Jerdonek's PR 19811. Thanks Chris!

[gvanrossum]

Oh, no! Buildbot failures. Help?!

[vstinner]

New changeset 3c7f9db by Victor Stinner in branch 'master':
Revert "bpo-29587: Enable implicit exception chaining with gen.throw() (GH-19811)" (bpo-19821)
3c7f9db