Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
bpo-29587: Enable implicit exception chaining with gen.throw()#19823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Before this commit, if an exception was active inside a generatorwhen calling gen.throw(), then that exception was lost (i.e. therewas no implicit exception chaining). This commit fixes that.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
test_unittest still crash on this PR. I tested on Linux (Fedora 32).
$ make clean$ make$ ./python -m test -v test_unittest (...)test_patch_nested_autospec_repr (unittest.test.testmock.testpatch.PatchTest) ... oktest_patch_object_keyword_args (unittest.test.testmock.testpatch.PatchTest) ... oktest_patch_object_with_spec_as_boolean (unittest.test.testmock.testpatch.PatchTest) ... oktest_patch_orderdict (unittest.test.testmock.testpatch.PatchTest) ... oktest_patch_propagates_exc_on_exit (unittest.test.testmock.testpatch.PatchTest) ... Fatal Python error: Segmentation faultCurrent thread 0x00007f886bd1c740 (most recent call first): File "/home/vstinner/python/master/Lib/unittest/mock.py", line 1524 in __exit__ File "/home/vstinner/python/master/Lib/unittest/test/testmock/testpatch.py", line 1673 in __exit__ File "/home/vstinner/python/master/Lib/contextlib.py", line 498 in __exit__ File "/home/vstinner/python/master/Lib/unittest/mock.py", line 1323 in decoration_helper File "/home/vstinner/python/master/Lib/contextlib.py", line 135 in __exit__ File "/home/vstinner/python/master/Lib/unittest/mock.py", line 1337 in patched File "/home/vstinner/python/master/Lib/unittest/case.py", line 201 in handle File "/home/vstinner/python/master/Lib/unittest/case.py", line 733 in assertRaises File "/home/vstinner/python/master/Lib/unittest/test/testmock/testpatch.py", line 1692 in test_patch_propagates_exc_on_exit(...)bedevere-bot commentedApr 30, 2020
When you're done making the requested changes, leave the comment: |
Is there a way to mark this PR as "don't merge" for now? This is the same PR as before, so it has the same issue. The Azure CI job seems not to be running, which is what exhibited the failure before.
Yes, this is the same failure as before. Everything passes locally on my Mac, so it will take me more time to look into this. |
vstinner commentedApr 30, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hum, I wasn't sure, so I did the 3 tricks that I know:
It seems like only the conversion to a draft technically prevents someone to merge the PR my mistake. |
Looks like that will cover it. ;) |
cjerdonek commentedMay 1, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Okay, I believe my latest changes fix the buildbot failures from before. Also, for future reference, below is a script I came up with that isolates the behavior difference between Mac and Fedora, with my prior version of the PR: importreimportsysdeff():exc,exc_value,tb=sys.exc_info()print(f'CLEARING FRAME:{tb.tb_frame!r}')tb.tb_frame.clear()defg():# Uncommenting the following line caused the tb_frame.clear() line# above to exhibit the following platform-specific behavior:# 1) On Mac, this is logged to stderr# > TypeError: print_exception(): Exception expected for value,# NoneType found# 2) On Fedora 32, the following error happens:# > Fatal Python error: Segmentation faultdata=re.compile('xxx')try:yieldexceptException:f()gen=g()gen.send(None)gen.throw(ValueError) Maybe this suggests an issue elsewhere in Python's code base. Should this PR also get a What's New? |
I have made the requested changes; please review again. |
bedevere-bot commentedMay 1, 2020
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. "./python -m test -j0 -r" does no longer crash on my Fedora.
I understood that passing NULL value to _PyErr_ChainExceptions() created the bug? Maybe add an assertion to prevent the situation to happen again?
Thank you. Regarding an assertion, the thing is that it looks like Would you still like me to add an assertion and/or perhaps a code comment explaining this possibility? |
bedevere-bot commentedMay 2, 2020
@cjerdonek: Please replace |
This is a follow-up toGH-19823 that removes the check that theexception value isn't NULL, prior to calling _PyErr_ChainExceptions().This enables implicit exception chaining for gen.throw() in morecircumstances.The commit also adds a test that a particular code snippet involvinggen.throw() doesn't crash. The test shows why the new`gi_exc_state.exc_type != Py_None` check that was added is necessary.Without the new check, the code snippet (as well as a number of othertests) crashes on certain platforms (e.g. Fedora but not Mac).
Uh oh!
There was an error while loading.Please reload this page.
This is a new version of PR#19811 to sort out the buildbot failures.
It enables implicit exception chaining when calling
generator.throw(exc)by settingexc.__context__.https://bugs.python.org/issue29587