Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
vstinner opened this issue Aug 17, 2023 · 6 comments
Closed

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

vstinner opened this issue Aug 17, 2023 · 6 comments
Labels
topic-C-API type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

vstinner commented Aug 17, 2023

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Proposal:

I added a private _PyErr_WriteUnraisableMsg() API but it cannot be used in third party code, nor in stdlib extensions which try to avoid private/internal functions (like sqlite3). Moreover, _PyErr_WriteUnraisableMsg() API doesn't allow to fully customize the error message.

The limitation of _PyErr_WriteUnraisableMsg() affected PR #106674 which has to add ; consider using ... in the error message which is not great.

        _PyErr_WriteUnraisableMsg(
            "in PyMapping_HasKeyString(); consider using "
            "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()",
            NULL);

@serhiy-storchaka suggested to add an API which allows string formatting, similar to PyErr_Format().

_PyErr_WriteUnraisableMsg() was added in issue #81010.


By the way, we should go through all calls to PyErr_WriteUnraisable() and add more context to more these logs easier to get for developers: give more context about what the issue is, how to fix it, where it occurred, etc.

Linked PRs

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 17, 2023

I already started to work on this, but was not sure about API, so left it to your return.

I initially was going to add simple function:

void PyErr_FormatUnraisable(const char *format, ...);

With special case PyErr_FormatUnraisable(NULL, obj) equal to PyErr_WriteUnraisable(obj).

But it now creates the UnraisableHookArgs object which has field object. So we need more complex API:

void PyErr_FormatUnraisable(PyObject *obj, const char *format, ...);

With PyErr_WriteUnraisable(obj) equal to PyErr_FormatUnraisable(obj, "Exception ignored in: %R", obj) or special case PyErr_FormatUnraisable(obj, NULL).

Or we always set UnraisableHookArgs.object to None if use PyErr_FormatUnraisable(), then we can make the use simpler. Then custom message and object will be mutually exclusive, but seems that currently most of cases with custom message pass NULL as object.

Or we can keep PyErr_FormatUnraisable() simpler and add a separate function PyErr_FormatUnraisableObject() which allows to specify both custom message and object.

void PyErr_FormatUnraisable(const char *format, ...);
void PyErr_FormatUnraisableObject(PyObject *obj, const char *format, ...);

@vstinner
Copy link
Member Author

vstinner commented Sep 4, 2023

I wrote PR #108863 to remove the private _PyErr_WriteUnraisableMsg() function from the public C API.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Oct 19, 2023
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Oct 29, 2023
Also document the behavior when called with NULL.
serhiy-storchaka added a commit that referenced this issue Oct 30, 2023
Also document the behavior when called with NULL.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 30, 2023
…GH-111455)

Also document the behavior when called with NULL.
(cherry picked from commit bca3305)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue Oct 30, 2023
…1455) (GH-111507)

Also document the behavior when called with NULL.
(cherry picked from commit bca3305)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Oct 31, 2023
Replace most of calls of _PyErr_WriteUnraisableMsg() and some
calls of PyErr_WriteUnraisable(NULL) with PyErr_FormatUnraisable().
serhiy-storchaka added a commit that referenced this issue Nov 2, 2023
Replace most of calls of _PyErr_WriteUnraisableMsg() and some
calls of PyErr_WriteUnraisable(NULL) with PyErr_FormatUnraisable().

Co-authored-by: Victor Stinner <vstinner@python.org>
@serhiy-storchaka
Copy link
Member

Few calls of _PyErr_WriteUnraisableMsg are left which pass both message and non-NULL object.

Modules/atexitmodule.c:140:            _PyErr_WriteUnraisableMsg("in atexit callback", the_func);
--
Modules/_threadmodule.c:1074:            _PyErr_WriteUnraisableMsg("in thread started by", boot->func);
--
Modules/_ctypes/callbacks.c:219:        _PyErr_WriteUnraisableMsg("on calling ctypes callback function",
Modules/_ctypes/callbacks.c-220-                                  callable);
--
Modules/_ctypes/callbacks.c:261:            _PyErr_WriteUnraisableMsg("on converting result "
Modules/_ctypes/callbacks.c-262-                                      "of ctypes callback function",
Modules/_ctypes/callbacks.c-263-                                      callable);
--
Modules/_ctypes/callbacks.c:273:                _PyErr_WriteUnraisableMsg("on converting result "
Modules/_ctypes/callbacks.c-274-                                          "of ctypes callback function",
Modules/_ctypes/callbacks.c-275-                                          callable);

They can be replaced with PyErr_FormatUnraisable(), but then object will be None. Or we can add new function specially for these cases, which passes both object and a format with arguments for a warning message. I do not know how important to have object.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Nov 2, 2023
Replace the remaining calls with PyErr_FormatUnraisable().
@vstinner
Copy link
Member Author

vstinner commented Nov 2, 2023

They can be replaced with PyErr_FormatUnraisable(), but then object will be None. Or we can add new function specially for these cases, which passes both object and a format with arguments for a warning message. I do not know how important to have object.

As the author of _PyErr_WriteUnraisableMsg(), I suggest to remove _PyErr_WriteUnraisableMsg() and replace all _PyErr_WriteUnraisableMsg() usage with PyErr_FormatUnraisable().

My intent for passing object to the hook was to delegate the formatting to give the hook the opportunity to catch exception on formatting the object, and maybe use tracemalloc to see where the object was allocated, etc.

But I don't think that these features are really useful and we should just skip/remove object, and just pass an error message to the hook.

@serhiy-storchaka
Copy link
Member

As the author of _PyErr_WriteUnraisableMsg(), I suggest to remove _PyErr_WriteUnraisableMsg() and replace all _PyErr_WriteUnraisableMsg() usage with PyErr_FormatUnraisable().

I created #111643 for this.

serhiy-storchaka added a commit that referenced this issue Nov 3, 2023
Replace the remaining calls with PyErr_FormatUnraisable().
FullteaR pushed a commit to FullteaR/cpython that referenced this issue Nov 3, 2023
FullteaR pushed a commit to FullteaR/cpython that referenced this issue Nov 3, 2023
Replace most of calls of _PyErr_WriteUnraisableMsg() and some
calls of PyErr_WriteUnraisable(NULL) with PyErr_FormatUnraisable().

Co-authored-by: Victor Stinner <vstinner@python.org>
FullteaR pushed a commit to FullteaR/cpython that referenced this issue Nov 3, 2023
Replace the remaining calls with PyErr_FormatUnraisable().
@serhiy-storchaka
Copy link
Member

Now we can replace more PyErr_Clear() with informative PyErr_FormatUnraisable().

aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
Replace most of calls of _PyErr_WriteUnraisableMsg() and some
calls of PyErr_WriteUnraisable(NULL) with PyErr_FormatUnraisable().

Co-authored-by: Victor Stinner <vstinner@python.org>
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
Replace the remaining calls with PyErr_FormatUnraisable().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants