classification
Title: Daemon threads must be forbidden in subinterpreters
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: aeros, eric.snow, ncoghlan, pitrou, tomMoral, vstinner
Priority: normal Keywords: patch

Created on 2019-06-13 09:36 by vstinner, last changed 2020-03-02 10:29 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
subinterp_daemon_thread.py vstinner, 2019-06-13 09:36
Pull Requests
URL Status Linked Edit
PR 14049 merged vstinner, 2019-06-13 10:07
PR 14584 merged vstinner, 2019-07-04 10:23
Messages (14)
msg345485 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-13 09:36
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.
msg345612 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-14 16:55
New changeset 066e5b1a917ec2134e8997d2cadd815724314252 by Victor Stinner in branch 'master':
bpo-37266: Daemon threads are now denied in subinterpreters (GH-14049)
https://github.com/python/cpython/commit/066e5b1a917ec2134e8997d2cadd815724314252
msg345613 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-14 16:56
Daemon threads must die. That's a first step towards their death!
msg347287 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-04 16:30
New changeset b4e68960b90627422325fdb75f463df1e4153c6e by Victor Stinner in branch 'master':
bpo-37266: Add bpo number to the What's New entry (GH614584)
https://github.com/python/cpython/commit/b4e68960b90627422325fdb75f463df1e4153c6e
msg361563 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-02-07 10:39
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'
msg361564 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-02-07 10:41
I reported the issue to jep: https://github.com/ninia/jep/issues/229
msg362890 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2020-02-28 18:11
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.
msg362891 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2020-02-28 18:13
Perhaps the solution would be to have an additional kind of atexit hooks, which get executed before threads are joined.
msg362911 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2020-02-28 19:53
Also cc'ing Kyle for the concurrent.futures issue.
msg362960 - (view) Author: Kyle Stanley (aeros) * (Python triager) Date: 2020-02-29 02:02
> 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()`.
msg362975 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2020-02-29 12:20
> 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.
msg363013 - (view) Author: Kyle Stanley (aeros) * (Python triager) Date: 2020-02-29 20:42
> 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).
msg363024 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-02-29 22:37
> There will be a problem with `concurrent.futures.ProcessPoolExecutor`, which currently launches its management thread as a daemon thread.

Please don't discuss in closed issues.

If you want to support concurrent.futures in subinterpreters, please open a separated RFE issue.
msg363149 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2020-03-02 10:29
Ok, I opened issue39812
History
Date User Action Args
2020-03-02 10:29:47pitrousetmessages: + msg363149
2020-02-29 22:37:27vstinnersetmessages: + msg363024
2020-02-29 20:42:39aerossetmessages: + msg363013
2020-02-29 12:20:11pitrousetmessages: + msg362975
2020-02-29 02:02:43aerossetmessages: + msg362960
2020-02-28 19:53:54pitrousetnosy: + aeros
messages: + msg362911
2020-02-28 18:13:32pitrousetnosy: + ncoghlan
messages: + msg362891
2020-02-28 18:11:44pitrousetnosy: + tomMoral, pitrou
messages: + msg362890
2020-02-07 10:41:45vstinnersetmessages: + msg361564
2020-02-07 10:39:19vstinnersetmessages: + msg361563
2019-07-04 16:30:42vstinnersetmessages: + msg347287
2019-07-04 10:23:34vstinnersetpull_requests: + pull_request14402
2019-06-14 16:56:51vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg345613

stage: patch review -> resolved
2019-06-14 16:55:27vstinnersetmessages: + msg345612
2019-06-13 10:07:07vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request13911
2019-06-13 10:05:26xtreaksetnosy: + eric.snow
2019-06-13 09:36:05vstinnercreate