gh-106672: C API: Report indiscriminately ignored errors #106674
Conversation
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_hookA 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. |
Oh sorry I miss it, thanks. |
|
Would it be possible to suggest a fix in the warning? Like using an API which report errors? |
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.
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?
|
|
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. |
I would prefer richer API, similar to |
That sounds like a good idea :-) |
Objects/dictobject.c
Outdated
| { | ||
| return dict_getitem(op, key, | ||
| "in PyDict_GetItem(); consider using " | ||
| "PyDict_GetItemWithError()"); |
There was a problem hiding this comment.
There will be new PyDict_GetItemRef().
Objects/dictobject.c
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
No replacement currently
8000
. There will be new PyDict_GetItemRefString().
Python/sysmodule.c
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
No replacement currently.
There was a problem hiding this comment.
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
Python/sysmodule.c
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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
Objects/abstract.c
Outdated
| if (rc == 0 && PyErr_Occurred()) { | ||
| _PyErr_WriteUnraisableMsg( | ||
| "in PyMapping_HasKeyString(); consider using " | ||
| "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()", |
There was a problem hiding this comment.
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()".
There was a problem hiding this comment.
Unfortunately _PyErr_WriteUnraisableMsg() does not support this.
There was a problem hiding this comment.
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? |
|
I could merge this PR long time ago, but I wanted to do some things before to make this PR better:
Of course it can be merged before implementing all steps above, but then we will need to return to this code to improve it. |
|
Updated, improved warning messages. Adding a replacement for |
Objects/abstract.c
Outdated
| if (v) { | ||
| Py_DECREF(v); | ||
| return 1; | ||
| PyObject *dummy; |
There was a problem hiding this comment.
Maybe rename to 'item' or 'value'.
|
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>
They are already mentioned in the documentation for corresponding functions. |
Objects/abstract.c
Outdated
| PyErr_FormatUnraisable( | ||
| "Ignore exception set before calling in PyMapping_HasKeyString(); " | ||
| "consider using PyMapping_HasKeyStringWithError(), " | ||
| "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is why it should be clearly documented.
Obviously you can call PyErr_Occurred(), PyErr_GetRaisedException() or PyErr_WriteUnraisable() when an exception is set.
…nGH-106674) Functions which indiscriminately ignore all errors now report them as unraisable errors.
…nGH-106674) Functions which indiscriminately ignore all errors now report them as unraisable errors.
…nGH-106674) Functions which indiscriminately ignore all errors now report them as unraisable errors.
Functions PyDict_GetItem(), PyDict_GetItemString(),
PyMapping_HasKey(), PyMapping_HasKeyString(),
PyObject_HasAttr(), PyObject_HasAttrString(), and
PySys_GetObject(), which clear all errors occurred during calling
the function, report now them using sys.unraisablehook().