msg345485 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
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) * |
Date: 2019-06-14 16:56 |
Daemon threads must die. That's a first step towards their death!
|
msg347287 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2020-02-28 19:53 |
Also cc'ing Kyle for the concurrent.futures issue.
|
msg362960 - (view) |
Author: Kyle Stanley (aeros) * |
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) * |
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) * |
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) * |
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) * |
Date: 2020-03-02 10:29 |
Ok, I opened issue39812
|
msg366030 - (view) |
Author: Eric Snow (eric.snow) * |
Date: 2020-04-08 23:18 |
I've opened bpo-40234 to address backward incompatibility from this
change (e.g. affecting mod-wsgi).
|
msg366269 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2020-04-12 21:45 |
New changeset 14d5331eb5e6c38be12bad421bd59ad0fac9e448 by Victor Stinner in branch 'master':
bpo-40234: Revert "bpo-37266: Daemon threads are now denied in subinterpreters (GH-14049)" (GH-19456)
https://github.com/python/cpython/commit/14d5331eb5e6c38be12bad421bd59ad0fac9e448
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:16 | admin | set | github: 81447 |
2020-04-12 21:45:20 | vstinner | set | messages:
+ msg366269 |
2020-04-10 14:14:17 | vstinner | set | pull_requests:
+ pull_request18811 |
2020-04-08 23:18:43 | eric.snow | set | messages:
+ msg366030 |
2020-03-02 10:29:47 | pitrou | set | messages:
+ msg363149 |
2020-02-29 22:37:27 | vstinner | set | messages:
+ msg363024 |
2020-02-29 20:42:39 | aeros | set | messages:
+ msg363013 |
2020-02-29 12:20:11 | pitrou | set | messages:
+ msg362975 |
2020-02-29 02:02:43 | aeros | set | messages:
+ msg362960 |
2020-02-28 19:53:54 | pitrou | set | nosy:
+ aeros messages:
+ msg362911
|
2020-02-28 18:13:32 | pitrou | set | nosy:
+ ncoghlan messages:
+ msg362891
|
2020-02-28 18:11:44 | pitrou | set | nosy:
+ tomMoral, pitrou messages:
+ msg362890
|
2020-02-07 10:41:45 | vstinner | set | messages:
+ msg361564 |
2020-02-07 10:39:19 | vstinner | set | messages:
+ msg361563 |
2019-07-04 16:30:42 | vstinner | set | messages:
+ msg347287 |
2019-07-04 10:23:34 | vstinner | set | pull_requests:
+ pull_request14402 |
2019-06-14 16:56:51 | vstinner | set | status: open -> closed resolution: fixed messages:
+ msg345613
stage: patch review -> resolved |
2019-06-14 16:55:27 | vstinner | set | messages:
+ msg345612 |
2019-06-13 10:07:07 | vstinner | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request13911 |
2019-06-13 10:05:26 | xtreak | set | nosy:
+ eric.snow
|
2019-06-13 09:36:05 | vstinner | create | |