gh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (in Objects/) #102218
Conversation
iritkatriel
added
skip news
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 09f2ab6 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
iritkatriel
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 0fef293 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
iritkatriel
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 9b5b62a 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
iritkatriel
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 6378d56 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
iritkatriel
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 0863f35 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
iritkatriel
added
the
🔨 test-with-refleak-buildbots
|
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 7a436c9 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
|
All the refleaks tests passed now. |
| PyErr_Fetch(&exc, &val, &tb); | ||
| caught_type = (PyTypeObject *)exc; | ||
| PyObject *exc = PyErr_GetRaisedException(); | ||
| PyTypeObject *caught_type = Py_TYPE(exc); | ||
| /* Ensure type info indicates no extra state is stored at the C level |
There was a problem hiding this comment.
Does all this stuff about "wrapping safely" make sense any more?
Since there are no "denomralized" exceptions, this should always work.
I think all this code is making sure that calling PyErr_NormalizeException is safe.
There was a problem hiding this comment.
Good question. Let me try to understand what's going on here.
There was a problem hiding this comment.
I don't think this is about PyErr_NormalizeException. This function is called from only one place:
/* Helper that tries to ensure the reported exception chain indicates the
* codec that was invoked to trigger the failure without changing the type
* of the exception raised.
*/
static void
wrap_codec_error(const char *operation,
const char *encoding)
{
/* TrySetFromCause will replace the active exception with a suitably
* updated clone if it can, otherwise it will leave the original
* exception alone.
*/
_PyErr_TrySetFromCause("%s with '%s' codec failed",
operation, encoding);
}
This is a classic use case for notes (PEP 678). It is trying to create an exception of the same type with additional info (and does a lot of stuff to check that it can indeed safely create a new exception of the same type).
I would just rewrite this whole thing to attach the additional info as a note (but not in this PR, it's unrelated).
There was a problem hiding this comment.
A PR for that is at: #102407
| /* While we could potentially copy a non-empty instance dictionary | ||
| * to the replacement exception, for now we take the more | ||
| * conservative path of leaving exceptions with attributes set | ||
| * alone. | ||
| */ | ||
| PyErr_Restore(exc, val, tb); | ||
| PyErr_SetRaisedException(exc); | ||
| return NULL; | ||
| } | ||
|
|
There was a problem hiding this comment.
This block comment should go as well (if I'm correct about removing the above code)
|
When you're done making the requested changes, leave the comment: |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
* main: pythongh-102304: Consolidate Direct Usage of _Py_RefTotal (pythongh-102514) pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (in Objects/) (python#102218) pythongh-102507 Remove invisible pagebreak characters (python#102531) pythongh-102515: Remove unused imports in the `Lib/` directory (python#102516) Remove or update bitbucket links (pythonGH-101963) pythongh-101100: Fix sphinx warnings in `zipapp` and `zipfile` modules (python#102526) pythonGH-102397: Fix segfault from race condition in signal handling (python#102399) Fix style in argparse.rst (python#101733) Post 3.12.0a6 fix typo in async generator code field name `ag_code` (python#102448) Python 3.12.0a6
(splitting up #102193 to make it more manageable).