Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-108082: Add PyErr_FormatUnraisable() function#111086
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
gh-108082: Add PyErr_FormatUnraisable() function#111086
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Variadic argument... cannot be used in some programming languages like Go, see:capi-workgroup/problems#35 Would it be possible to addalso a variant which takes an already formatted string?
Something like:PyErr_WriteUnraisableMsg(PyObject *msg) (I'm not sure about the name, nor the API).
| Similar to :c:func:`PyErr_WriteUnraisable`, but the *format* and subsequent | ||
| parameters help format the warning message; they have the same meaning and | ||
| values as in :c:func:`PyUnicode_FromFormat`. | ||
| If *format* is ``NULL``, only the traceback is printed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would you mind to give examples? I'm not sure how to use this API.
I understood that PyErr_WriteUnraisable(obj) can be written as:
PyErr_FormatUnraisable("Exception ignored in: %R",obj);
And withoutan exception an object, it should be:
PyErr_FormatUnraisable("Exception ignored in:");
Do the format must end with a newline character?
Ifformat is
NULL, only the traceback is printed.
What's the usage of this mode? Is the caller expected to log something like "Exception ignored in xxx" manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PyErr_FormatUnraisable(NULL) is the same asPyErr_WriteUnraisable(NULL). It just prints the traceback, without "Exception ignored ...".
I simply documented the existing behavior forPyErr_WriteUnraisable(), and forPyErr_FormatUnraisable(NULL) the most natural way is to do the same. What alternative do you propose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, my remark is not specific to NULL value, but more about the whole function. I don't know which string to pass to the function. I suggest to add an example to the doc. Like:
For example, ``PyErr_WriteUnraisable(obj)`` is similar to ``PyErr_FormatUnraisable("Exception ignored in: %R", obj);``.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would it be possible to not add this function to the limited C API in Python 3.13, but wait for Python 3.14?
What if change |
| Similar to :c:func:`PyErr_WriteUnraisable`, but the *format* and subsequent | ||
| parameters help format the warning message; they have the same meaning and | ||
| values as in :c:func:`PyUnicode_FromFormat`. | ||
| If *format* is ``NULL``, only the traceback is printed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PyErr_FormatUnraisable(NULL) is the same asPyErr_WriteUnraisable(NULL). It just prints the traceback, without "Exception ignored ...".
I simply documented the existing behavior forPyErr_WriteUnraisable(), and forPyErr_FormatUnraisable(NULL) the most natural way is to do the same. What alternative do you propose?
| @@ -270,6 +274,104 @@ def test_setfromerrnowithfilename(self): | |||
| (ENOENT, 'No such file or directory', 'file')) | |||
| # CRASHES setfromerrnowithfilename(ENOENT, NULL, b'error') | |||
| def test_err_writeunraisable(self): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TODO: This test should be backported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See#111455.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Overall, the change LGTM. I just suggest to add a small example in the doc. I also propose to change how errors are handled, but that can be done later, and I can propose a followu-p PR for that if you want.
| Similar to :c:func:`PyErr_WriteUnraisable`, but the *format* and subsequent | ||
| parameters help format the warning message; they have the same meaning and | ||
| values as in :c:func:`PyUnicode_FromFormat`. | ||
| If *format* is ``NULL``, only the traceback is printed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, my remark is not specific to NULL value, but more about the whole function. I don't know which string to pass to the function. I suggest to add an example to the doc. Like:
For example, ``PyErr_WriteUnraisable(obj)`` is similar to ``PyErr_FormatUnraisable("Exception ignored in: %R", obj);``.| self.assertEqual(str(cm.unraisable.exc_value), 'oops!') | ||
| self.assertEqual(cm.unraisable.exc_traceback.tb_lineno, | ||
| firstline + 15) | ||
| self.assertIsNone(cm.unraisable.err_msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would prefer to log an error in the error handler in this case. Ignoring silently errors doesn't help :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
_PyErr_WriteUnraisableMsg() does the same: silently ignores decoding and memory errors.
I minimized changes. Other changes can be made in a separate PR.
Python/errors.c Outdated
| _PyErr_WriteUnraisableMsg(const char *err_msg_str, PyObject *obj) | ||
| static void | ||
| format_unraisable_v(PyObject *obj, const char *format, va_list va) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would it be possible to move obj to end, last parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure. But I would make the order of arguments inconsistent informat_unraisable() andformat_unraisable_v().
| if (err_msg_str != NULL) { | ||
| err_msg =PyUnicode_FromFormat("Exception ignored %s", err_msg_str); | ||
| if (format != NULL) { | ||
| err_msg =PyUnicode_FromFormatV(format, va); | ||
| if (err_msg == NULL) { | ||
| PyErr_Clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think that it's the best behavior. I would prefer togoto error. What do you think?
MaybePyUnicode_FromString("Exception ignored in sys.unraisablehook") can become a_Py_DECLARE_STR(exc_ignored_msg, "...") +_Py_STR(exc_ignored_msg) static immortal string to avoid error 3 while handling error 2 while handling error 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is the same as in_PyErr_WriteUnraisableMsg(). Changing the behavior of_PyErr_WriteUnraisableMsg() is out of the scope of this PR.
The main limitation of the new function is that it doesn't allow to pass an object. But I'm not sure that it's really useful. Maybe we should just deprecated this attribute of sys.unraisablehook. I don't know. I mostly added it to delegate the string formatting to the hook. |
Only 6 of about 70 use cases in CPython code actually pass an object. If it is needed, we can add yet one function, but the function which do not require to pass mandatory NULL argument in 90% of cases is more convenient. |
Thanks for the short example in the doc ;-) |
Uh oh!
There was an error while loading.Please reload this page.
📚 Documentation preview 📚:https://cpython-previews--111086.org.readthedocs.build/