Subinterpreters don't properly clean up threads

[ZeroIntensity]

Crash report

What happened?

I found this issue a little while back, but I'm finally getting around to fixing it.

Currently, subinterpreter finalization assumes that there's only one thread left. So, any remaining threads--daemon or non-daemon--crash the interpreter upon finalizing:

import _interpreters

interp = _interpreters.create()
source = """
import threading
import time

def hello():
    time.sleep(1)

t = threading.Thread(target=hello)
t.start()
"""
_interpreters.run_string(interp, source)

This results in an assertion failure on my end:

python: Python/pystate.c:1969: tstate_activate: Assertion `!tstate->_status.bound_gilstate || tstate == gilstate_tss_get((tstate->interp->runtime))' failed.

So, there's a few things that need to get fixed:

• The finalization process shouldn't assume that there's one thread. Instead, it should just dive straight into Py_EndInterpreter, which will properly clean things up.
• It also shouldn't try to manually delete threads via attaching to them and then calling PyThreadState_Delete. That should also happen in Py_EndInterpreter, after all threads have finished.
• Subinterpreter finalization itself happens way too late. _PyRuntimeState_SetFinalizing has been set, meaning that all threads will be already blocked, and thus cannot shutdown via threading._shutdown, resulting in a deadlock. finalize_subinterpreters should be called before that happens.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Output from running 'python -VV' on the command line:

No response

Linked PRs

• gh-128639: Don't assume one thread in subinterpreter finalization #128640
• [3.14] gh-128639: Don't assume one thread in subinterpreter finalization (gh-128640) #134254
• Revert "gh-128639: Don't assume one thread in subinterpreter finalization (gh-128640)" #134256
• gh-128639: Don't assume one thread in subinterpreter finalization with fixed daemon thread support #134606

[ericsnowcurrently]

FWIW, I thought I had created an issue about cleaning up lingering thread states automatically. Would you mind double-checking and closing it, if so?

[ZeroIntensity]

For those who aren't me or Eric: we had to revert the fix because it breaks the test suite when run with --single-process. I'm still trying to figure out why.

[ZeroIntensity]

@ericsnowcurrently I finally figured out the buildbot failures. This is an awesome bug.

Here's a repro that crashes with my patch applied:

import _interpreters
import types
import time

interp = _interpreters.create(
    types.SimpleNamespace(
        use_main_obmalloc=False,
        allow_fork=False,
        allow_exec=False,
        allow_threads=True,
        allow_daemon_threads=True,
        check_multi_interp_extensions=True,
        gil='own',
    )
)
_interpreters.exec(interp, f"""if True:
    import threading
    import time

    def task():
        time.sleep(1)

    threads = [threading.Thread(target=task, daemon=True) for _ in range(3)]
    for t in threads:
        t.start()
    """)
_interpreters.destroy(interp)
time.sleep(2)

The problem is that something subtly changed with daemon thread shutdown in-between writing my patch and merging it. So, after the time.sleep() call, the thread would still be alive and crash. Unfortunately for me, I randomly chose 100 seconds as the sleep time, so if the process exited within those 100 seconds, nothing would crash. I'll put up the fixed version.