Skip to content
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

Daemon threads must be forbidden in subinterpreters #81447

Closed
vstinner opened this issue Jun 13, 2019 · 16 comments
Closed

Daemon threads must be forbidden in subinterpreters #81447

vstinner opened this issue Jun 13, 2019 · 16 comments
Labels
3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@vstinner
Copy link
Member

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
    Copy link
    Member Author

    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 vstinner added 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jun 13, 2019
    @vstinner
    Copy link
    Member Author

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

    @vstinner
    Copy link
    Member Author

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

    @vstinner
    Copy link
    Member Author

    vstinner commented Jul 4, 2019

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

    @vstinner
    Copy link
    Member Author

    vstinner commented Feb 7, 2020

    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
    Copy link
    Member Author

    vstinner commented Feb 7, 2020

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

    @pitrou
    Copy link
    Member

    pitrou commented Feb 28, 2020

    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
    Copy link
    Member

    pitrou commented Feb 28, 2020

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

    @pitrou
    Copy link
    Member

    pitrou commented Feb 28, 2020

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

    @aeros
    Copy link
    Contributor

    aeros commented Feb 29, 2020

    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
    Copy link
    Member

    pitrou commented Feb 29, 2020

    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
    Copy link
    Contributor

    aeros commented Feb 29, 2020

    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).

    @vstinner
    Copy link
    Member Author

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 2, 2020

    Ok, I opened bpo-39812

    @ericsnowcurrently
    Copy link
    Member

    I've opened bpo-40234 to address backward incompatibility from this
    change (e.g. affecting mod-wsgi).

    @vstinner
    Copy link
    Member Author

    New changeset 14d5331 by Victor Stinner in branch 'master':
    bpo-40234: Revert "bpo-37266: Daemon threads are now denied in subinterpreters (GH-14049)" (GH-19456)
    14d5331

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants