Daemon threads must be forbidden in subinterpreters

[vstinner]

BPO	37266
Nosy	@ncoghlan, @pitrou, @vstinner, @ericsnowcurrently, @tomMoral, @aeros
PRs	bpo-37266: Daemon threads are now denied in subinterpreters #14049 bpo-37266: Add bpo number to the What's New entry #14584 bpo-40234: Revert "bpo-37266: Daemon threads are now denied in subinterpreters (GH-14049)" #19456
Files	subinterp_daemon_thread.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-06-14.16:56:51.832>
created_at = <Date 2019-06-13.09:36:05.704>
labels = ['interpreter-core', '3.9']
title = 'Daemon threads must be forbidden in subinterpreters'
updated_at = <Date 2020-04-12.21:45:20.499>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2020-04-12.21:45:20.499>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2019-06-14.16:56:51.832>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2019-06-13.09:36:05.704>
creator = 'vstinner'
dependencies = []
files = ['48417']
hgrepos = []
issue_num = 37266
keywords = ['patch']
message_count = 16.0
messages = ['345485', '345612', '345613', '347287', '361563', '361564', '362890', '362891', '362911', '362960', '362975', '363013', '363024', '363149', '366030', '366269']
nosy_count = 6.0
nosy_names = ['ncoghlan', 'pitrou', 'vstinner', 'eric.snow', 'tomMoral', 'aeros']
pr_nums = ['14049', '14584', '19456']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue37266'
versions = ['Python 3.9']

[vstinner]

Py_EndInterpreter() calls threading._shutdown() which waits for non-daemon threads spawned in the subinterpreters. Problem: daemon threads continue to run after threading._shutdown(), but they rely on an interpreter which is being finalized and then deleted.

Attached example shows the problem:

$ ./python subinterp_daemon_thread.py 
hello from daemon thread
Fatal Python error: Py_EndInterpreter: not the last thread

Current thread 0x00007f13e5926740 (most recent call first):
File "subinterp_daemon_thread.py", line 23 in <module>
Aborted (core dumped)

Catching the bug in Py_EndInterpreter() is too late. IMHO we must simply deny daemon threads by design in subinterpreters for safety.

In the main interpreter, we provide best effort to prevent crash at exit, but IMHO the implementation is ugly :-( ceval.c uses exit_thread_if_finalizing(): it immediately exit the current daemon thread if the threads attempts to acquire or release the GIL, whereas the interpreter is gone. Problem: we cannot release/clear some data structure at Python exit because of that. So Py_Finalize() may leak some memory by design, because of daemon threads.

IMHO we can be way stricter in subinterpreters.

I suggest to only modify Python 3.9.

[vstinner]

New changeset 066e5b1 by Victor Stinner in branch 'master':
bpo-37266: Daemon threads are now denied in subinterpreters (GH-14049)
066e5b1

[vstinner]

Daemon threads must die. That's a first step towards their death!

[vstinner]

New changeset b4e6896 by Victor Stinner in branch 'master':
bpo-37266: Add bpo number to the What's New entry (GH614584)
b4e6896

[vstinner]

FYI python-jep project is broken by this change:
https://bugzilla.redhat.com/show_bug.cgi?id=1792062

"JEP embeds CPython in Java through JNI and is safe to use in a heavily threaded environment."
https://github.com/ninia/jep

FAIL: test_shared_modules_threads (test_shared_modules.TestSharedModules)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/builddir/build/BUILD/jep-3.9.0/src/test/python/test_shared_modules.py", line 15, in test_shared_modules_threads
    jep_pipe(build_java_process_cmd('jep.test.TestSharedModulesThreads'))
  File "/usr/lib64/python3.9/contextlib.py", line 240, in helper
    return _GeneratorContextManager(func, args, kwds)
  File "/usr/lib64/python3.9/contextlib.py", line 83, in __init__
    self.gen = func(*args, **kwds)
  File "/builddir/build/BUILD/jep-3.9.0/src/test/python/jep_pipe.py", line 36, in jep_pipe
    assert False, stderr
AssertionError: b'Exception in thread "main" jep.JepException: <class \'RuntimeError\'>: daemon thread are not supported in subinterpreters\n\tat /usr/lib64/python3.9/threading.start(threading.py:858)\n\tat <string>.<module>(<string>:1)\n\tat jep.Jep.eval(Native Method)\n\tat jep.Jep.eval(Jep.java:451)\n\tat jep.test.TestSharedModulesThreads.main(TestSharedModulesThreads.java:53)\n'

[vstinner]

I reported the issue to jep: ninia/jep#229

[pitrou]

There will be a problem with concurrent.futures.ProcessPoolExecutor, which currently launches its management thread as a daemon thread. The daemon thread itself is not problematic, because ProcessPoolExecutor uses an atexit hook to shutdown itself and therefore join the management thread.

It seems, however, that it's not easy to make the thread non-daemon, because atexit hooks are executed *after* non-daemon threads are joined. That would lead to a deadlock: the interpreter would wait for the non-daemon management thread to exit, but the ProcessPoolExecutor would wait for the atexit hook to be called before telling the management thread to exit.

cc'ing Thomas Moreau, who's worker a lot on this.

[pitrou]

Perhaps the solution would be to have an additional kind of atexit hooks, which get executed before threads are joined.

[pitrou]

Also cc'ing Kyle for the concurrent.futures issue.

[aeros]

The daemon thread itself is not problematic, because ProcessPoolExecutor uses an atexit hook to shutdown itself and therefore join the management thread.

ThreadPoolExecutor also uses an atexit hook for its shutdown process. Also, it sets each worker thread to a daemon. So we'd definitely have to address as well that prior to killing off daemon threads.

Perhaps the solution would be to have an additional kind of atexit hooks, which get executed before threads are joined.

Hmm, a potential way to do this might be adding a form of "atexit hook" support that's specific to threads. Each registered function would get called in the internal _shutdown() [1] function in the threading module, just before all of the non-daemon threads are joined. To me, this seems best implemented as a new public function for the threading module, perhaps something like threading.register_atexit(). Would this be reasonable?

---

[1] - IIUC, threading._shutdown() is called in pylifecycle.c, in wait_for_thread_shutdown(), which is the way that non-daemon threads are joined when the interpreter shuts down in Py_EndInterpreter() or is finalized in Py_FinalizeEx().

[pitrou]

To me, this seems best implemented as a new public function for the threading module, perhaps something like threading.register_atexit(). Would this be reasonable?

To me, yes.

[aeros]

To me, yes.

If Victor is also on-board with the idea, I can look into writing a patch for it (after testing to verify that it allows us to change the daemon threads to normal threads in concurrent.futures without issues).