gh-106672: C API: Report indiscriminately ignored errors by serhiy-storchaka · Pull Request #106674 · python/cpython (original) (raw)
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 our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation33 Commits18 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Functions which indiscriminately ignore all errors now report them as unraisable errors.
Can you please list affected functions?
Is there a way to mute these warnings if an application is affected by this change but the maintainer is not available in the short term to fix these issues? These APIs were ignoring silently errors for years.
Maybe we need a command line option and/or environment variable to set an unraisable hook which... Does nothing.
In code, I suppose that it's easy to do:
import sys def silence_errors(args): pass sys.unraisablehook=silence_errors
I made a similar change in Python 3.13 io module: close() errors are now logged.
I listed them in the NEWS entry. Will add them to the commit message too.
There are no new reports in the CPython tests, so at least CPython code is almost clear. They can still occur if you press Ctrl-C or run program in memory constricted environment, but it is very unlikely.
I think that the problem of silencing unraisable exception messages is a separate issue. It may be even not too important issue if such messages are rare. In addition to "silence error" I would consider adding option for "abort immediately".
Oh sure, you can abort the process on the first unraisable exception:
import signal, sys
def abort_hook(unraisable): signal.raise_signal(signal.SIGABRT)
sys.unraisablehook = abort_hook
A better implementation may want to log the exception before 😁 Maybe by calling the old hook.
At the beginning, it was proposed to always crash the process.
Are you suggesting command line and env var to get his behavior?
pytest now catchs these unraisable exceptions.
I listed them in the NEWS entry
Oh sorry I miss it, thanks.
Would it be possible to suggest a fix in the warning? Like using an API which report errors?
Are you suggesting command line and env var to get his behavior?
It is a different issue, but I would be interested in such feature. Not that I need it now, but if I need it, it would be useful.
Would it be possible to suggest a fix in the warning? Like using an API which report errors?
On one hand, it would help the author of the extension. On other hand, it will overwhelm the user of the extension with not useful information. Should I keep the current more general description?
Exception ignored on testing a mapping key:Exception ignored in PyMapping_HasKey(); use PyMapping_GetOptionalItem() or PyObject_GetItem() instead:Exception ignored on testing a mapping key in PyMapping_HasKey(); use PyMapping_GetOptionalItem() or PyObject_GetItem() instead:
I would prefer to suggest using a different function to avoid such warning. About the phrasing, you may write "...; consider using xxx()".
I'm also fine with the shorter error message.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated note: we should consider making _PyErr_WriteUnraisableMsg() a public function. But i'm not sure about its API. Is the error message format easy to get?
Unrelated note: we should consider making _PyErr_WriteUnraisableMsg() a public function.
I would prefer richer API, similar to PyErr_Format().
I would prefer richer API, similar to PyErr_Format().
That sounds like a good idea :-)
| { |
|---|
| return dict_getitem(op, key, |
| "in PyDict_GetItem(); consider using " |
| "PyDict_GetItemWithError()"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be new PyDict_GetItemRef().
| @@ -3889,10 +3901,12 @@ PyDict_GetItemString(PyObject *v, const char *key) |
|---|
| PyObject *kv, *rv; |
| kv = PyUnicode_FromString(key); |
| if (kv == NULL) { |
| PyErr_Clear(); |
| _PyErr_WriteUnraisableMsg( |
| "in PyDict_GetItemString()", NULL); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No replacement currently. There will be new PyDict_GetItemRefString().
| @@ -98,6 +98,9 @@ PySys_GetObject(const char *name) |
|---|
| PyObject *value = _PySys_GetObject(tstate->interp, name); |
| /* XXX Suppress a new exception if it was raised and restore |
| * the old one. */ |
| if (_PyErr_Occurred(tstate)) { |
| _PyErr_WriteUnraisableMsg("in PySys_GetObject()", NULL); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No replacement currently.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm waiting until PyDict_GetItemRef() is accepted to consider proposing a variant for PySys_GetObject(). I'm tracking functions returning borrowed references and replacement at: https://pythoncapi.readthedocs.io/bad_api.html#functions
| @@ -98,6 +98,9 @@ PySys_GetObject(const char *name) |
|---|
| PyObject *value = _PySys_GetObject(tstate->interp, name); |
| /* XXX Suppress a new exception if it was raised and restore |
| * the old one. */ |
| if (_PyErr_Occurred(tstate)) { |
| _PyErr_WriteUnraisableMsg("in PySys_GetObject()", NULL); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm waiting until PyDict_GetItemRef() is accepted to consider proposing a variant for PySys_GetObject(). I'm tracking functions returning borrowed references and replacement at: https://pythoncapi.readthedocs.io/bad_api.html#functions
| if (rc == 0 && PyErr_Occurred()) { |
|---|
| _PyErr_WriteUnraisableMsg( |
| "in PyMapping_HasKeyString(); consider using " |
| "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case is very subtle and the error message is not heplful. It's non-obvious to me that the exception was already set before the function was called.
Maybe the error message should be something like: "Ignore exception set before calling in PyMapping_HasKeyString()".
Instead of "Exception ignored in PyMapping_HasKeyString()".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately _PyErr_WriteUnraisableMsg() does not support this. ☹️
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It may annoy people but there is a way to silence these warnings, so i'm fine with it.
Error messages can be enhanced, but the current API to log unraisable exception is too limited. It's ok the revisit them later.
I dislike ignoring errors, it can silence important errors. So this change is a nice step forward.
Do you need help to solve conflicts? I approved your PR but you didn't merge it yet. Do you plan further changes?
Updated, improved warning messages.
Adding a replacement for PySys_GetObject() is more controversial, so it is better to merge this PR first.
| if (v) { |
|---|
| Py_DECREF(v); |
| return 1; |
| PyObject *dummy; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename to 'item' or 'value'.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
If these warnings become a performance issue later, maybe we can compute some fixed strings as static strings, like _Py_STR() API.
In the What's New, would it be possible to mention recommended replacement function(s), maybe as a list (func: replace)?
Co-authored-by: Victor Stinner vstinner@python.org
In the What's New, would it be possible to mention recommended replacement function(s), maybe as a list (func: replace)?
They are already mentioned in the documentation for corresponding functions.
| PyErr_FormatUnraisable( |
|---|
| "Ignore exception set before calling in PyMapping_HasKeyString(); " |
| "consider using PyMapping_HasKeyStringWithError(), " |
| "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these 3 functions be called with an exception set? They don't override the exception? That sounds surprising. I would prefer suggesting to not call the function with an exception set. What do you think?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got catch. Indeed, PyMapping_GetOptionalItemString() can only return 0 if no exception is set, so this condition is always false. Also, the alternative functions also can clear exceptions.
I think that we should classify the C API by classes:
- Function that can be called when an exception is set, and they do not change it.
- Function that can be called when an exception is set, and they do not change it unless they fail.
- Function that can be called when an exception is set, but they can change it even at success.
- Function that can be called when an exception is set, but the result is ambiguous in some cases (you cannot distinguish some successful results from failure).
- Function that should never be called when an exception is set.
There may be more classes.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the general case, to write safe code handling a raised exception, I think the safest option is to keep the exception aside using PyErr_GetRaisedException(). Maybe today some functions are perfectly fine and never override the currently raised exception. But what if tomorrow their implementation changes, and they may start to clear the currently raised exception?
In Python, in an except: block, there is no "currently raised exception" in the C level, even if sys.exc_info() returns the exception. The difference between PyThreadState.exc_info and PyThreadState.current_exception is subtle.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why it should be clearly documented.
Obviously you can call PyErr_Occurred(), PyErr_GetRaisedException() or PyErr_WriteUnraisable() when an exception is set.
hugovk pushed a commit to hugovk/cpython that referenced this pull request
Functions which indiscriminately ignore all errors now report them as unraisable errors.
aisk pushed a commit to aisk/cpython that referenced this pull request
Functions which indiscriminately ignore all errors now report them as unraisable errors.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request
Functions which indiscriminately ignore all errors now report them as unraisable errors.
