Skip to content

bpo-36829: Add _PyErr_WriteUnraisableMsg() #13488

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

Merged
merged 1 commit into from
May 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Doc/library/sys.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1566,11 +1566,16 @@ always available.
* *exc_type*: Exception type.
* *exc_value*: Exception value, can be ``None``.
* *exc_traceback*: Exception traceback, can be ``None``.
* *err_msg*: Error message, can be ``None``.
* *object*: Object causing the exception, can be ``None``.

:func:`sys.unraisablehook` can be overridden to control how unraisable
exceptions are handled.

The default hook formats *err_msg* and *object* as:
``f'{err_msg}: {object!r}'``; use "Exception ignored in" error message
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
``f'{err_msg}: {object!r}'``; use "Exception ignored in" error message
``f'Exception ignored {err_msg}: {object!r}'``; use "Exception ignored in" error message

Copy link
Member Author

Choose a reason for hiding this comment

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

It really logs f'{err_msg}: {object!r}'. _PyErr_WriteUnraisableMsg("xxx") sets err_msg to "Exception ignored xxx".

if *err_msg* is ``None``.

See also :func:`excepthook` which handles uncaught exceptions.

.. versionadded:: 3.8
Expand Down
3 changes: 3 additions & 0 deletions Include/cpython/pyerrors.h
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,9 @@ PyAPI_FUNC(PyObject *) _PyUnicodeTranslateError_Create(
const char *reason /* UTF-8 encoded string */
);

PyAPI_FUNC(void) _PyErr_WriteUnraisableMsg(
const char *err_msg,
PyObject *obj);

#ifdef __cplusplus
}
Expand Down
44 changes: 27 additions & 17 deletions Lib/test/test_sys.py
Original file line number Diff line number Diff line change
Expand Up @@ -878,31 +878,38 @@ def test__enablelegacywindowsfsencoding(self):

@test.support.cpython_only
class UnraisableHookTest(unittest.TestCase):
def write_unraisable_exc(self, exc, obj):
def write_unraisable_exc(self, exc, err_msg, obj):
import _testcapi
import types
err_msg2 = f"Exception ignored {err_msg}"
try:
_testcapi.write_unraisable_exc(exc, obj)
_testcapi.write_unraisable_exc(exc, err_msg, obj)
return types.SimpleNamespace(exc_type=type(exc),
exc_value=exc,
exc_traceback=exc.__traceback__,
err_msg=err_msg2,
object=obj)
finally:
# Explicitly break any reference cycle
exc = None

def test_original_unraisablehook(self):
obj = "an object"

with test.support.captured_output("stderr") as stderr:
with test.support.swap_attr(sys, 'unraisablehook',
sys.__unraisablehook__):
self.write_unraisable_exc(ValueError(42), obj)

err = stderr.getvalue()
self.assertIn(f'Exception ignored in: {obj!r}\n', err)
self.assertIn('Traceback (most recent call last):\n', err)
self.assertIn('ValueError: 42\n', err)
for err_msg in (None, "original hook"):
with self.subTest(err_msg=err_msg):
obj = "an object"

with test.support.captured_output("stderr") as stderr:
with test.support.swap_attr(sys, 'unraisablehook',
sys.__unraisablehook__):
self.write_unraisable_exc(ValueError(42), err_msg, obj)

err = stderr.getvalue()
if err_msg is not None:
self.assertIn(f'Exception ignored {err_msg}: {obj!r}\n', err)
else:
self.assertIn(f'Exception ignored in: {obj!r}\n', err)
self.assertIn('Traceback (most recent call last):\n', err)
self.assertIn('ValueError: 42\n', err)

def test_original_unraisablehook_err(self):
# bpo-22836: PyErr_WriteUnraisable() should give sensible reports
Expand Down Expand Up @@ -962,8 +969,9 @@ def hook_func(args):
obj = object()
try:
with test.support.swap_attr(sys, 'unraisablehook', hook_func):
expected = self.write_unraisable_exc(ValueError(42), obj)
for attr in "exc_type exc_value exc_traceback object".split():
expected = self.write_unraisable_exc(ValueError(42),
"custom hook", obj)
for attr in "exc_type exc_value exc_traceback err_msg object".split():
self.assertEqual(getattr(hook_args, attr),
getattr(expected, attr),
(hook_args, expected))
Expand All @@ -978,10 +986,12 @@ def hook_func(*args):

with test.support.captured_output("stderr") as stderr:
with test.support.swap_attr(sys, 'unraisablehook', hook_func):
self.write_unraisable_exc(ValueError(42), None)
self.write_unraisable_exc(ValueError(42),
"custom hook fail", None)

err = stderr.getvalue()
self.assertIn(f'Exception ignored in: {hook_func!r}\n',
self.assertIn(f'Exception ignored in sys.unraisablehook: '
f'{hook_func!r}\n',
err)
self.assertIn('Traceback (most recent call last):\n', err)
self.assertIn('Exception: hook_func failed\n', err)
Expand Down
6 changes: 3 additions & 3 deletions Modules/_ctypes/_ctypes.c
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,9 @@ _DictRemover_call(PyObject *myself, PyObject *args, PyObject *kw)
{
DictRemoverObject *self = (DictRemoverObject *)myself;
if (self->key && self->dict) {
if (-1 == PyDict_DelItem(self->dict, self->key))
/* XXX Error context */
PyErr_WriteUnraisable(Py_None);
if (-1 == PyDict_DelItem(self->dict, self->key)) {
_PyErr_WriteUnraisableMsg("on calling _ctypes.DictRemover", NULL);
}
Py_CLEAR(self->key);
Py_CLEAR(self->dict);
}
Expand Down
17 changes: 14 additions & 3 deletions Modules/_testcapimodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -4985,13 +4985,24 @@ negative_refcount(PyObject *self, PyObject *Py_UNUSED(args))
static PyObject*
test_write_unraisable_exc(PyObject *self, PyObject *args)
{
PyObject *exc, *obj;
if (!PyArg_ParseTuple(args, "OO", &exc, &obj)) {
PyObject *exc, *err_msg, *obj;
if (!PyArg_ParseTuple(args, "OOO", &exc, &err_msg, &obj)) {
return NULL;
}

const char *err_msg_utf8;
if (err_msg != Py_None) {
err_msg_utf8 = PyUnicode_AsUTF8(err_msg);
if (err_msg_utf8 == NULL) {
return NULL;
}
}
else {
err_msg_utf8 = NULL;
}

PyErr_SetObject((PyObject *)Py_TYPE(exc), exc);
PyErr_WriteUnraisable(obj);
_PyErr_WriteUnraisableMsg(err_msg_utf8, obj);
Py_RETURN_NONE;
}

Expand Down
5 changes: 2 additions & 3 deletions Modules/gcmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -929,9 +929,8 @@ delete_garbage(struct _gc_runtime_state *state,
Py_INCREF(op);
(void) clear(op);
if (PyErr_Occurred()) {
PySys_WriteStderr("Exception ignored in tp_clear of "
"%.50s\n", Py_TYPE(op)->tp_name);
PyErr_WriteUnraisable(NULL);
_PyErr_WriteUnraisableMsg("in tp_clear of",
(PyObject*)Py_TYPE(op));
}
Py_DECREF(op);
}
Expand Down
9 changes: 5 additions & 4 deletions Python/clinic/sysmodule.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

84 changes: 71 additions & 13 deletions Python/errors.c
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,7 @@ static PyStructSequence_Field UnraisableHookArgs_fields[] = {
{"exc_type", "Exception type"},
{"exc_value", "Exception value"},
{"exc_traceback", "Exception traceback"},
{"err_msg", "Error message"},
{"object", "Object causing the exception"},
{0}
};
Expand All @@ -1085,7 +1086,7 @@ static PyStructSequence_Desc UnraisableHookArgs_desc = {
.name = "UnraisableHookArgs",
.doc = UnraisableHookArgs__doc__,
.fields = UnraisableHookArgs_fields,
.n_in_sequence = 4
.n_in_sequence = 5
};


Expand All @@ -1104,7 +1105,8 @@ _PyErr_Init(void)

static PyObject *
make_unraisable_hook_args(PyThreadState *tstate, PyObject *exc_type,
PyObject *exc_value, PyObject *exc_tb, PyObject *obj)
PyObject *exc_value, PyObject *exc_tb,
PyObject *err_msg, PyObject *obj)
{
PyObject *args = PyStructSequence_New(&UnraisableHookArgsType);
if (args == NULL) {
Expand All @@ -1125,6 +1127,7 @@ make_unraisable_hook_args(PyThreadState *tstate, PyObject *exc_type,
ADD_ITEM(exc_type);
ADD_ITEM(exc_value);
ADD_ITEM(exc_tb);
ADD_ITEM(err_msg);
ADD_ITEM(obj);
#undef ADD_ITEM

Expand All @@ -1145,11 +1148,21 @@ make_unraisable_hook_args(PyThreadState *tstate, PyObject *exc_type,
static int
write_unraisable_exc_file(PyThreadState *tstate, PyObject *exc_type,
PyObject *exc_value, PyObject *exc_tb,
PyObject *obj, PyObject *file)
PyObject *err_msg, PyObject *obj, PyObject *file)
{
if (obj != NULL && obj != Py_None) {
if (PyFile_WriteString("Exception ignored in: ", file) < 0) {
return -1;
if (err_msg != NULL && err_msg != Py_None) {
if (PyFile_WriteObject(err_msg, file, Py_PRINT_RAW) < 0) {
return -1;
}
if (PyFile_WriteString(": ", file) < 0) {
return -1;
}
}
else {
if (PyFile_WriteString("Exception ignored in: ", file) < 0) {
return -1;
}
}

if (PyFile_WriteObject(obj, file, 0) < 0) {
Expand All @@ -1162,6 +1175,14 @@ write_unraisable_exc_file(PyThreadState *tstate, PyObject *exc_type,
return -1;
}
}
else if (err_msg != NULL && err_msg != Py_None) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can avoid duplication of the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to have a single PyFile_WriteString(":\n", file) call in this code path, rather than trying to factorize the code too much, to reduce the risk of failure in the hook.

if (PyFile_WriteObject(err_msg, file, Py_PRINT_RAW) < 0) {
return -1;
}
if (PyFile_WriteString(":\n", file) < 0) {
return -1;
}
}

if (exc_tb != NULL && exc_tb != Py_None) {
if (PyTraceBack_Print(exc_tb, file) < 0) {
Expand All @@ -1178,8 +1199,9 @@ write_unraisable_exc_file(PyThreadState *tstate, PyObject *exc_type,
const char *className = PyExceptionClass_Name(exc_type);
if (className != NULL) {
const char *dot = strrchr(className, '.');
if (dot != NULL)
if (dot != NULL) {
className = dot+1;
}
}

_Py_IDENTIFIER(__module__);
Expand Down Expand Up @@ -1238,7 +1260,8 @@ write_unraisable_exc_file(PyThreadState *tstate, PyObject *exc_type,

static int
write_unraisable_exc(PyThreadState *tstate, PyObject *exc_type,
PyObject *exc_value, PyObject *exc_tb, PyObject *obj)
PyObject *exc_value, PyObject *exc_tb, PyObject *err_msg,
PyObject *obj)
{
PyObject *file = _PySys_GetObjectId(&PyId_stderr);
if (file == NULL || file == Py_None) {
Expand All @@ -1249,7 +1272,7 @@ write_unraisable_exc(PyThreadState *tstate, PyObject *exc_type,
while we use it */
Py_INCREF(file);
int res = write_unraisable_exc_file(tstate, exc_type, exc_value, exc_tb,
obj, file);
err_msg, obj, file);
Py_DECREF(file);

return res;
Expand All @@ -1272,9 +1295,10 @@ _PyErr_WriteUnraisableDefaultHook(PyObject *args)
PyObject *exc_type = PyStructSequence_GET_ITEM(args, 0);
PyObject *exc_value = PyStructSequence_GET_ITEM(args, 1);
PyObject *exc_tb = PyStructSequence_GET_ITEM(args, 2);
PyObject *obj = PyStructSequence_GET_ITEM(args, 3);
PyObject *err_msg = PyStructSequence_GET_ITEM(args, 3);
PyObject *obj = PyStructSequence_GET_ITEM(args, 4);

if (write_unraisable_exc(tstate, exc_type, exc_value, exc_tb, obj) < 0) {
if (write_unraisable_exc(tstate, exc_type, exc_value, exc_tb, err_msg, obj) < 0) {
return NULL;
}
Py_RETURN_NONE;
Expand All @@ -1287,13 +1311,18 @@ _PyErr_WriteUnraisableDefaultHook(PyObject *args)
for Python to handle it. For example, when a destructor raises an exception
or during garbage collection (gc.collect()).

If err_msg_str is non-NULL, the error message is formatted as:
"Exception ignored %s" % err_msg_str. Otherwise, use "Exception ignored in"
error message.

An exception must be set when calling this function. */
void
PyErr_WriteUnraisable(PyObject *obj)
_PyErr_WriteUnraisableMsg(const char *err_msg_str, PyObject *obj)
{
PyThreadState *tstate = _PyThreadState_GET();
assert(tstate != NULL);

PyObject *err_msg = NULL;
PyObject *exc_type, *exc_value, *exc_tb;
_PyErr_Fetch(tstate, &exc_type, &exc_value, &exc_tb);

Expand Down Expand Up @@ -1322,13 +1351,20 @@ PyErr_WriteUnraisable(PyObject *obj)
}
}

if (err_msg_str != NULL) {
err_msg = PyUnicode_FromFormat("Exception ignored %s", err_msg_str);
Copy link
Member

Choose a reason for hiding this comment

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

Hummm, why don't allowing the caller to control the full error message? (Instead of prepending "Exception ignored")

Copy link
Member Author

Choose a reason for hiding this comment

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

@serhiy-storchaka asked me to do so :-) And when I started a draft change to convert existing PyErr_WriteUnraisable() to _PyErr_WriteUnraisableMsg(), I always started error messages with "Exception ignored ".

The hook allows to control the full error message. Only proposed _PyErr_WriteUnraisableMsg() enfoce "Exception ignored " prefix. We might add a new function later if needed to let control the full error message.

Copy link
Member Author

@vstinner vstinner May 26, 2019

Choose a reason for hiding this comment

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

By the way, making "Exception ignored " prefix part of the API should reduce the size of the Python executable binary since constant strings are shorter (remove common "Exception ignored " prefix ;-)).

if (err_msg == NULL) {
PyErr_Clear();
}
}

_Py_IDENTIFIER(unraisablehook);
PyObject *hook = _PySys_GetObjectId(&PyId_unraisablehook);
if (hook != NULL && hook != Py_None) {
PyObject *hook_args;

hook_args = make_unraisable_hook_args(tstate, exc_type, exc_value,
exc_tb, obj);
exc_tb, err_msg, obj);
if (hook_args != NULL) {
PyObject *args[1] = {hook_args};
PyObject *res = _PyObject_FastCall(hook, args, 1);
Expand All @@ -1337,6 +1373,18 @@ PyErr_WriteUnraisable(PyObject *obj)
Py_DECREF(res);
goto done;
}

err_msg_str = "Exception ignored in sys.unraisablehook";
}
else {
err_msg_str = ("Exception ignored on building "
"sys.unraisablehook arguments");
}

Py_XDECREF(err_msg);
err_msg = PyUnicode_FromString(err_msg_str);
if (err_msg == NULL) {
PyErr_Clear();
}

/* sys.unraisablehook failed: log its error using default hook */
Expand All @@ -1350,15 +1398,25 @@ PyErr_WriteUnraisable(PyObject *obj)

default_hook:
/* Call the default unraisable hook (ignore failure) */
(void)write_unraisable_exc(tstate, exc_type, exc_value, exc_tb, obj);
(void)write_unraisable_exc(tstate, exc_type, exc_value, exc_tb,
err_msg, obj);

done:
Py_XDECREF(exc_type);
Py_XDECREF(exc_value);
Py_XDECREF(exc_tb);
Py_XDECREF(err_msg);
_PyErr_Clear(tstate); /* Just in case */
}


void
PyErr_WriteUnraisable(PyObject *obj)
{
_PyErr_WriteUnraisableMsg(NULL, obj);
}


extern PyObject *PyModule_GetWarningsModule(void);


Expand Down
Loading