Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchakaserhiy-storchaka commentedJul 12, 2023
edited
Loading

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

Functions which indiscriminately ignore all errors now report them asunraisable errors.
@vstinner
Copy link
Member

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:

importsysdefsilence_errors(args):passsys.unraisablehook=silence_errors
olife-png reacted with thumbs up emoji

@vstinner
Copy link
Member

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

olife-png reacted with thumbs up emoji

@serhiy-storchaka
Copy link
MemberAuthor

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
Copy link
Member

Oh sure, you can abort the process on the first unraisable exception:

importsignal,sysdefabort_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
Copy link
Member

I listed them in the NEWS entry

Oh sorry I miss it, thanks.

@vstinner
Copy link
Member

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

@serhiy-storchaka
Copy link
MemberAuthor

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
Copy link
Member

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.

Copy link
Member

@vstinnervstinner left a comment

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?

@serhiy-storchaka
Copy link
MemberAuthor

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

I would prefer richer API, similar toPyErr_Format().

@vstinner
Copy link
Member

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

That sounds like a good idea :-)

@ye-abcye-abc mentioned this pull requestJul 12, 2023
{
return dict_getitem(op, key,
"in PyDict_GetItem(); consider using "
"PyDict_GetItemWithError()");
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There will be newPyDict_GetItemRef().

vstinner reacted with thumbs up emoji
@@ -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);
Copy link
MemberAuthor

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 newPyDict_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);
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

No replacement currently.

Copy link
Member

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);
Copy link
Member

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()",
Copy link
Member

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

Copy link
MemberAuthor

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.☹️

Copy link
Member

@vstinnervstinner left a comment

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.

@vstinner
Copy link
Member

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
Copy link
MemberAuthor

serhiy-storchaka commentedAug 26, 2023
edited
Loading

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.

@serhiy-storchaka
Copy link
MemberAuthor

Updated, improved warning messages.

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

if (v) {
Py_DECREF(v);
return 1;
PyObject *dummy;
Copy link
Member

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done.

@vstinner
Copy link
Member

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

@vstinner
Copy link
Member

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
Copy link
MemberAuthor

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()");
Copy link
Member

@vstinnervstinnerNov 6, 2023
edited
Loading

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?

Copy link
MemberAuthor

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:

  1. Function that can be called when an exception is set, and they do not change it.
  2. Function that can be called when an exception is set, and they do not change it unless they fail.
  3. Function that can be called when an exception is set, but they can change it even at success.
  4. 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).
  5. Function that should never be called when an exception is set.

There may be more classes.

Copy link
Member

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 usingPyErr_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 anexcept: block, there is no "currently raised exception" in the C level, even ifsys.exc_info() returns the exception. The difference betweenPyThreadState.exc_info andPyThreadState.current_exception is subtle.

Copy link
MemberAuthor

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 callPyErr_Occurred(),PyErr_GetRaisedException() orPyErr_WriteUnraisable() when an exception is set.

@serhiy-storchakaserhiy-storchaka merged commitf55cb44 intopython:mainNov 7, 2023
@serhiy-storchakaserhiy-storchaka deleted the capi-write-unraisable-for-ignored-errors branchNovember 7, 2023 13:58
hugovk pushed a commit to hugovk/cpython that referenced this pull requestNov 8, 2023
…nGH-106674)Functions which indiscriminately ignore all errors now report them asunraisable errors.
aisk pushed a commit to aisk/cpython that referenced this pull requestFeb 11, 2024
…nGH-106674)Functions which indiscriminately ignore all errors now report them asunraisable errors.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull requestSep 2, 2024
…nGH-106674)Functions which indiscriminately ignore all errors now report them asunraisable errors.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner approved these changes

@methanemethaneAwaiting requested review from methanemethane is a code owner

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@serhiy-storchaka@vstinner@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp