bpo-1230540: Add threading.excepthook() #13515

vstinner · 2019-05-23T00:02:01Z

Add a new threading.excepthook() function which handles uncaught
Thread.run() exception. It can be overridden to control how uncaught
exceptions are handled.

threading.ExceptHookArgs is not documented on purpose: it should not
be used directly.

threading.excepthook() and threading.ExceptHookArgs.
Add _PyErr_Display(): similar to PyErr_Display(), but accept a
'file' parameter.
Add _thread._excepthook(): C implementation of the exception hook
calling _PyErr_Display().
Add _thread._ExceptHookArgs: structseq type.
Add threading._invoke_excepthook_wrapper() which handles the gory
details to ensure that everything remains alive during Python
shutdown.
Add unit tests.

https://bugs.python.org/issue1230540

vstinner · 2019-05-23T00:02:31Z

cc @vlasovskikh

vstinner · 2019-05-23T00:03:29Z

See PR #8610 for a different approach: reuse sys.excepthook.

vstinner · 2019-05-23T00:10:21Z

In my current implementation, I chose to continue to rely on t_bootstrap() of Modules/_threadmodule.c to log exception in threading.excepthook().

Maybe another option would be to attempt to reuse sys.excepthook() to log threading.excepthook() new failure?

Maybe the best option would be to later reimplement threading.excepthook() in C which reduces a lot the risk of error during Python shutdown. In C, it's more reliable to call functions during Python shutdown. Especially functions which don't rely on Python modules, but only access the interpreter state, thread state and the current exception.

I prefer to start with a simple PR to introduce the new API, and later experiment the different options to make the code even more reliable.

Doc/library/sys.rst

vstinner · 2019-05-25T00:31:16Z

I rewrote my PR, see the details in the issue: https://bugs.python.org/issue1230540#msg343443

vstinner · 2019-05-25T00:57:03Z

Oops, I didn't revert properly my change which added "stderr" into arguments passed to threading.excepthook(). It's now fixed.

vstinner · 2019-05-25T09:44:10Z

@serhiy-storchaka @pablogsal @pitrou: Would you mind to review the updated PR? See https://bugs.python.org/issue1230540 for the discussion.

pitrou · 2019-05-25T17:32:43Z

From the doc: Handle uncaught :func:``Thread.run`` exception.

What if some C extension wants to report uncaught exceptions in C-created threads? Does it have to create a dummy Thread object?

For example, how about threads created by _thread.start_new_thread?

vstinner · 2019-05-25T23:39:25Z

I modified my PR to use the threading identifier (threading.get_ident()) as the thread name if args.thread is None.

vstinner · 2019-05-26T00:00:22Z

As you can see in test_excepthook_thread_None(), it's not possible to call directly threading.excepthook() directly only using the public API: threading._make_excepthook_args() is needed.

args = threading._make_excepthook_args([*sys.exc_info(), None])

Should it be made public? Maybe as threading.ExceptHookArgs?

Maybe it can be made public but not documented?

Add a new threading.excepthook() function which handles uncaught Thread.run() exception. It can be overridden to control how uncaught exceptions are handled. threading.ExceptHookArgs is not documented on purpose: it should not be used directly. * threading.excepthook() and threading.ExceptHookArgs. * Add _PyErr_Display(): similar to PyErr_Display(), but accept a 'file' parameter. * Add _thread._excepthook(): C implementation of the exception hook calling _PyErr_Display(). * Add _thread._ExceptHookArgs: structseq type. * Add threading._invoke_excepthook_wrapper() which handles the gory details to ensure that everything remains alive during Python shutdown. * Add unit tests.

vstinner · 2019-05-27T07:32:16Z

I squashed my commits, rebased my PR on master and I renamed _make_excepthook_args as threading.ExceptHookArgs.

I chose to not document ExceptHookArgs. In the Python implementation, threading.ExceptHookArgs is a wrapper to threading._ExceptHookArgs: threading._ExceptHookArgs constructor takes N arguments, whereas ExceptHookArgs takes 1 argument.

Using threading.ExceptHookArgs, it's now possible to call directly threading.excepthook(threading.ExceptHook(...)) to log an error from a thread not created using threading.Thread, but using directly _thread.start_new_thread.

@pitrou: Does it look better to you?

vstinner · 2019-05-27T15:47:36Z

I plan to merge this PR wednesday (in 2 days).

I don't see a strong opposition, and nobody came up with a working implementation for the idea of reusing sys.excepthook.

There is PR #8610 but I dislike it: threading.Thread doesn't call sys.excepthook to handle run() exception by default, it only calls sys.excepthook if it's overridden. Moreover, when sys.excepthook is called, the hook doesn't get access to the thread object :-(
https://bugs.python.org/issue1230540#msg343260

asvetlov · 2019-05-27T15:58:56Z

Lib/threading.py

+    from traceback import print_exception as _print_exception
+    from collections import namedtuple
+
+    _ExceptHookArgs = namedtuple(


Is namedtuple for possible future extending a list of provided arguments?

Exactly. See sys.unraisablehook: I used the same approach and today I added a new "err_msg" attribute ;-)

asvetlov · 2019-05-27T16:08:50Z

Python/pythonrun.c

@@ -953,10 +953,11 @@ print_exception_recursive(PyObject *f, PyObject *value, PyObject *seen)
 }

 void
-PyErr_Display(PyObject *exception, PyObject *value, PyObject *tb)
+_PyErr_Display(PyObject *file, PyObject *exception, PyObject *value, PyObject *tb)


Make it public maybe? PyErr_DisplayToFile or something like this?

I would prefer to not add a new function to the public C API unless there is a clear need from users. In the past, the C API evolved organically and now it's a mess because nobody knows what is really used or not :-(

pitrou

Looks mostly good to me. Some comments below.

Doc/library/threading.rst

Doc/library/sys.rst

Modules/_threadmodule.c

bedevere-bot · 2019-05-27T16:37:50Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner · 2019-05-27T21:46:34Z

I have made the requested changes; please review again.

bedevere-bot · 2019-05-27T21:46:37Z

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

vstinner · 2019-05-27T22:45:35Z

Thanks for the review @pitrou!

vstinner · 2019-05-28T10:31:50Z

For example, how about threads created by _thread.start_new_thread?

I created https://bugs.python.org/issue37076 for this specific function ;-)

Add a new threading.excepthook() function which handles uncaught Thread.run() exception. It can be overridden to control how uncaught exceptions are handled. threading.ExceptHookArgs is not documented on purpose: it should not be used directly. * threading.excepthook() and threading.ExceptHookArgs. * Add _PyErr_Display(): similar to PyErr_Display(), but accept a 'file' parameter. * Add _thread._excepthook(): C implementation of the exception hook calling _PyErr_Display(). * Add _thread._ExceptHookArgs: structseq type. * Add threading._invoke_excepthook_wrapper() which handles the gory details to ensure that everything remains alive during Python shutdown. * Add unit tests.

the-knights-who-say-ni added the CLA signed label May 23, 2019

bedevere-bot added the awaiting core review label May 23, 2019

vstinner requested review from pitrou and serhiy-storchaka May 23, 2019 00:02

graingert reviewed May 24, 2019

View reviewed changes

Doc/library/sys.rst Outdated Show resolved Hide resolved

auvipy approved these changes May 25, 2019

View reviewed changes

vstinner mentioned this pull request May 27, 2019

bpo-1230540: Invoke custom sys.excepthook for threads in threading #8610

Closed

asvetlov reviewed May 27, 2019

View reviewed changes

pitrou requested changes May 27, 2019

View reviewed changes

bedevere-bot removed the awaiting core review label May 27, 2019

bedevere-bot added the awaiting changes label May 27, 2019

vstinner added 2 commits May 27, 2019 23:43

Address Antoine's review

5626165

Add missing PyErr_Clear()

8d60559

bedevere-bot added awaiting change review and removed awaiting changes labels May 27, 2019

pitrou approved these changes May 27, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 27, 2019

vstinner merged commit cd590a7 into python:master May 27, 2019

bedevere-bot removed the awaiting merge label May 27, 2019

vstinner deleted the threading_excepthook branch May 27, 2019 22:45

lazka mentioned this pull request May 28, 2019

hook threading.excepthook getsentry/sentry-python#376

Closed

sarnikowski mentioned this pull request Oct 28, 2024

Question: Logging of uncaught exceptions when running under a threadpool executor temporalio/sdk-python#677

Closed

Uh oh!

bpo-1230540: Add threading.excepthook() #13515

bpo-1230540: Add threading.excepthook() #13515

Uh oh!

Conversation

vstinner commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented May 23, 2019

Uh oh!

vstinner commented May 23, 2019

Uh oh!

vstinner commented May 23, 2019

Uh oh!

Uh oh!

vstinner commented May 25, 2019

Uh oh!

vstinner commented May 25, 2019

Uh oh!

vstinner commented May 25, 2019

Uh oh!

pitrou commented May 25, 2019

Uh oh!

vstinner commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented May 26, 2019

Uh oh!

vstinner commented May 27, 2019

Uh oh!

vstinner commented May 27, 2019

Uh oh!

asvetlov May 27, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner May 27, 2019

Choose a reason for hiding this comment

Uh oh!

asvetlov May 27, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner May 27, 2019

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented May 27, 2019

Uh oh!

vstinner commented May 27, 2019

Uh oh!

bedevere-bot commented May 27, 2019

Uh oh!

vstinner commented May 27, 2019

Uh oh!

vstinner commented May 28, 2019

Uh oh!

Uh oh!

vstinner commented May 23, 2019 •

edited

Loading

vstinner commented May 25, 2019 •

edited

Loading