gh-106672: C API: Report indiscriminately ignored errors #106674

serhiy-storchaka · 2023-07-12T09:20:57Z

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().

Issue: C API: Report ignored exception in C API functions which ignore all errors #106672

Functions which indiscriminately ignore all errors now report them as unraisable errors.

vstinner · 2023-07-12T10:10:54Z

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

vstinner · 2023-07-12T10:11:42Z

I made a similar change in Python 3.13 io module: close() errors are now logged.

serhiy-storchaka · 2023-07-12T10:37:31Z

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".

vstinner · 2023-07-12T11:28:53Z

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.

vstinner · 2023-07-12T11:29:26Z

I listed them in the NEWS entry

Oh sorry I miss it, thanks.

vstinner · 2023-07-12T11:31:11Z

Would it be possible to suggest a fix in the warning? Like using an API which report errors?

serhiy-storchaka · 2023-07-12T13:25:30Z

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:

vstinner · 2023-07-12T13:29:12Z

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.

Misc/NEWS.d/next/C API/2023-07-12-12-14-52.gh-issue-106672.fkRjmi.rst

vstinner

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?

serhiy-storchaka · 2023-07-12T14:37:45Z

Unrelated note: we should consider making _PyErr_WriteUnraisableMsg() a public function.

I would prefer richer API, similar to PyErr_Format().

vstinner · 2023-07-12T14:51:08Z

I would prefer richer API, similar to PyErr_Format().

That sounds like a good idea :-)

serhiy-storchaka · 2023-07-12T15:54:01Z

Objects/dictobject.c

+{
+    return dict_getitem(op, key,
+            "in PyDict_GetItem(); consider using "
+            "PyDict_GetItemWithError()");


There will be new PyDict_GetItemRef().

serhiy-storchaka · 2023-07-12T15:54:17Z

Objects/dictobject.c

@@ -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);


No replacement currently. There will be new PyDict_GetItemRefString().

serhiy-storchaka · 2023-07-12T15:55:33Z

Python/sysmodule.c

@@ -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);


No replacement currently.

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

vstinner · 2023-07-12T16:00:55Z

Python/sysmodule.c

@@ -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);


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

vstinner · 2023-07-12T16:06:22Z

Objects/abstract.c

+    if (rc == 0 && PyErr_Occurred()) {
+        _PyErr_WriteUnraisableMsg(
+            "in PyMapping_HasKeyString(); consider using "
+            "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()",


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()".

Unfortunately _PyErr_WriteUnraisableMsg() does not support this. ☹️

vstinner

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.

vstinner · 2023-08-26T02:17:57Z

Do you need help to solve conflicts? I approved your PR but you didn't merge it yet. Do you plan further changes?

serhiy-storchaka · 2023-08-26T05:58:03Z

I could merge this PR long time ago, but I wanted to do some things before to make this PR better:

Add tests for PyObject_HasAttr(), PyMapping_HasKey(), etc. (Add the C API tests for Mapping Protocol and Sequence Protocol #107178)
Add tests for PySys_GetObject(). (Add C API tests for PySys_GetObject() and PySys_SetObject() #107735)
Enhance PyErr_WriteUnraisable() API. ([C API] Enhance PyErr_WriteUnraisable() API to pass an error message #108082)
Add replacements for PyObject_HasAttr(), PyMapping_HasKey(), etc. (C API: Add replacements for PyObject_HasAttr() etc #108511)
Add a replacement for PySys_GetObject(). (C API: Add a replacement for PySys_GetObject #108512)

Of course it can be merged before implementing all steps above, but then we will need to return to this code to improve it.

serhiy-storchaka · 2023-11-05T09:37:40Z

Updated, improved warning messages.

Adding a replacement for PySys_GetObject() is more controversial, so it is better to merge this PR first.

Objects/abstract.c

vstinner · 2023-11-06T14:51:21Z

Objects/abstract.c

-    if (v) {
-        Py_DECREF(v);
-        return 1;
+    PyObject *dummy;


Maybe rename to 'item' or 'value'.

vstinner · 2023-11-06T14:52:52Z

If these warnings become a performance issue later, maybe we can compute some fixed strings as static strings, like _Py_STR() API.

vstinner · 2023-11-06T14:54:00Z

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>

serhiy-storchaka · 2023-11-06T15:53:19Z

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.

vstinner · 2023-11-06T16:12:48Z

Objects/abstract.c

+        PyErr_FormatUnraisable(
+            "Ignore exception set before calling in PyMapping_HasKeyString(); "
+            "consider using PyMapping_HasKeyStringWithError(), "
+            "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()");


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?

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.

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.

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.

pythongh-106672: C API: Report indiscriminately ignored errors

e134bad

Functions which indiscriminately ignore all errors now report them as unraisable errors.

serhiy-storchaka requested a review from vstinner July 12, 2023 09:20

serhiy-storchaka requested review from methane and markshannon as code owners July 12, 2023 09:20

bedevere-bot added the awaiting core review label Jul 12, 2023

bedevere-bot mentioned this pull request Jul 12, 2023

C API: Report ignored exception in C API functions which ignore all errors #106672

Closed

vstinner reviewed Jul 12, 2023

View reviewed changes

Misc/NEWS.d/next/C API/2023-07-12-12-14-52.gh-issue-106672.fkRjmi.rst Show resolved Hide resolved

vstinner reviewed Jul 12, 2023

View reviewed changes

ye-abc mentioned this pull request Jul 12, 2023

Powerfull #106681

Closed

serhiy-storchaka added 2 commits July 12, 2023 18:42

Update output messsages.

00ce038

Add an entry in What's New.

168f92a

serhiy-storchaka commented Jul 12, 2023

View reviewed changes

vstinner reviewed Jul 12, 2023

View reviewed changes

Merge branch 'main' into capi-write-unraisable-for-ignored-errors

884da66

vstinner approved these changes Jul 12, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 12, 2023

serhiy-storchaka added 3 commits August 7, 2023 20:08

Merge branch 'main' into capi-write-unraisable-for-ignored-errors

58dbac5

Fix smelly symbol.

07c8250

Remove PyDict_GetItem(dict, invalid_key) from test_dict_capi().

56e3a49

serhiy-storchaka added 3 commits August 7, 2023 22:33

Merge branch 'main' into capi-write-unraisable-for-ignored-errors

4e13132

Fix regressions in handling NULLs.

b5a661b

Merge branch 'main' into capi-write-unraisable-for-ignored-errors

ab5eb8f

vstinner mentioned this pull request Aug 17, 2023

[C API] Enhance PyErr_WriteUnraisable() API to pass an error message #108082

Closed

serhiy-storchaka added 5 commits August 26, 2023 09:00

Merge branch 'main' into capi-write-unraisable-for-ignored-errors

c681ce2

Fix merging error.

82efd1c

Merge branch 'main' into capi-write-unraisable-for-ignored-errors

c687ff8

Add more suggestions.

310ae51

Improve error messages for ignoring errors set before the calls.

241576a

vstinner reviewed Nov 6, 2023

View reviewed changes

serhiy-storchaka and others added 2 commits November 6, 2023 17:44

Apply suggestions from code review

fd50ea9

Co-authored-by: Victor Stinner <vstinner@python.org>

Address review comments.

587b3cc

vstinner reviewed Nov 6, 2023

View reviewed changes

Remove non-existing cases.

bfb206a

serhiy-storchaka merged commit f55cb44 into python:main Nov 7, 2023
30 checks passed

bedevere-app bot removed the awaiting merge label Nov 7, 2023

serhiy-storchaka deleted the capi-write-unraisable-for-ignored-errors branch November 7, 2023 13:58

hugovk pushed a commit to hugovk/cpython that referenced this pull request Nov 8, 2023

pythongh-106672: C API: Report indiscriminately ignored errors (pytho…

c1001c5

…nGH-106674) Functions which indiscriminately ignore all errors now report them as unraisable errors.

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-106672: C API: Report indiscriminately ignored errors (pytho…

643f0b6

…nGH-106674) Functions which indiscriminately ignore all errors now report them as unraisable errors.

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-106672: C API: Report indiscriminately ignored errors (pytho…

42bf264

…nGH-106674) Functions which indiscriminately ignore all errors now report them as unraisable errors.

sklam mentioned this pull request Oct 23, 2024

Outdated PyObject_HasAttr documentation (Exception behavior) #125887

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-106672: C API: Report indiscriminately ignored errors #106674

gh-106672: C API: Report indiscriminately ignored errors #106674

serhiy-storchaka commented Jul 12, 2023 •

edited

Loading

vstinner commented Jul 12, 2023

vstinner commented Jul 12, 2023

serhiy-storchaka commented Jul 12, 2023

vstinner commented Jul 12, 2023

vstinner commented Jul 12, 2023

vstinner commented Jul 12, 2023

serhiy-storchaka commented Jul 12, 2023

vstinner commented Jul 12, 2023

vstinner left a comment

serhiy-storchaka commented Jul 12, 2023

vstinner commented Jul 12, 2023

serhiy-storchaka Jul 12, 2023

serhiy-storchaka Jul 12, 2023

serhiy-storchaka Jul 12, 2023

vstinner Jul 12, 2023

vstinner Jul 12, 2023

vstinner Jul 12, 2023

serhiy-storchaka Jul 12, 2023

vstinner left a comment

vstinner commented Aug 26, 2023

serhiy-storchaka commented Aug 26, 2023 •

edited

Loading

serhiy-storchaka commented Nov 5, 2023

vstinner Nov 6, 2023

serhiy-storchaka Nov 6, 2023

vstinner commented Nov 6, 2023

vstinner commented Nov 6, 2023

serhiy-storchaka commented Nov 6, 2023

vstinner Nov 6, 2023 •

edited

Loading

serhiy-storchaka Nov 6, 2023

vstinner Nov 7, 2023

serhiy-storchaka Nov 7, 2023

gh-106672: C API: Report indiscriminately ignored errors #106674

gh-106672: C API: Report indiscriminately ignored errors #106674

Conversation

serhiy-storchaka commented Jul 12, 2023 • edited Loading

vstinner commented Jul 12, 2023

vstinner commented Jul 12, 2023

serhiy-storchaka commented Jul 12, 2023

vstinner commented Jul 12, 2023

vstinner commented Jul 12, 2023

vstinner commented Jul 12, 2023

serhiy-storchaka commented Jul 12, 2023

vstinner commented Jul 12, 2023

vstinner left a comment

Choose a reason for hiding this comment

serhiy-storchaka commented Jul 12, 2023

vstinner commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Aug 26, 2023

serhiy-storchaka commented Aug 26, 2023 • edited Loading

serhiy-storchaka commented Nov 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Nov 6, 2023

vstinner commented Nov 6, 2023

serhiy-storchaka commented Nov 6, 2023

vstinner Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented Jul 12, 2023 •

edited

Loading

serhiy-storchaka commented Aug 26, 2023 •

edited

Loading

vstinner Nov 6, 2023 •

edited

Loading