bpo-45635: Do not suppress errors in functions called from _PyErr_Display #30073
Conversation
iritkatriel
changed the title
iritkatriel
changed the title
iritkatriel
changed the title
Makes sense to me. |
erlend-aasland
left a comment
erlend-aasland
left a comment
There was a problem hiding this comment.
Looks very good! I want to go over this once more in a day or two, if that's ok with you.
|
Of course, this is not urgent at all. |
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
erlend-aasland
left a comment
erlend-aasland
left a comment
There was a problem hiding this comment.
LGTM, but AFAICT we are unable to actually test this because we must fail in something like PyFile_WriteString (or OOM) for print_exception_recursive to actually return -1. I'm wondering if we could provoke a write error in some kind of way.
| // the end of that one since we don't support multi-line error | ||
| // messages. | ||
| if (end_lineno > lineno) { | ||
| end_offset = (error_line != NULL) ? line_size : -1; |
There was a problem hiding this comment.
This branch is not covered by the test suite, AFAICS.
There was a problem hiding this comment.
Maybe make a bpo about this? I just copied this piece of code around, so it's not a new problem.
There was a problem hiding this comment.
This code is fairly recent:
commit a77aac4
Author: Pablo Galindo [email protected]
Date: Fri Apr 23 14:27:05 2021 +0100
There was a problem hiding this comment.
If the test is added in a separate PR it can be backported to 3.10.
iritkatriel
left a comment
iritkatriel
left a comment
There was a problem hiding this comment.
I was also thinking how one could test these error conditions. I think we would require some debug-mode-only macro that fails on the Nth time, and then run the test for all relevant values of N. It could be quite slow.
| // the end of that one since we don't support multi-line error | ||
| // messages. | ||
| if (end_lineno > lineno) { | ||
| end_offset = (error_line != NULL) ? line_size : -1; |
There was a problem hiding this comment.
Maybe make a bpo about this? I just copied this piece of code around, so it's not a new problem.
iritkatriel
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit d6f6bbb 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
4 participants
Previously print_exception_recursive, and some other functions, called PyErr_Clear before returning. Now each of these functions returns the error so that we go back to the topmost _PyErr_Display and the PyErr_Clear happens there. So the whole thing exits, rather than just this one recursive call.
https://bugs.python.org/issue45635