From 79ed70ef727579551a14f97c5e39b36a621fcb97 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 13:37:58 -0400 Subject: [PATCH] Revert "gh-128639: Don't assume one thread in subinterpreter finalization (gh-128640)" This reverts commit 9859791f9e116c827468f307ac0770286c975c8b. --- Lib/test/test_interpreters/test_api.py | 60 +------------------ Lib/test/test_interpreters/test_lifecycle.py | 6 +- Lib/test/test_threading.py | 5 +- ...-01-08-12-52-47.gh-issue-128640.9nbh9z.rst | 1 - Programs/_testembed.c | 9 +-- Python/pylifecycle.c | 56 ++++++++--------- 6 files changed, 38 insertions(+), 99 deletions(-) delete mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-01-08-12-52-47.gh-issue-128640.9nbh9z.rst diff --git a/Lib/test/test_interpreters/test_api.py b/Lib/test/test_interpreters/test_api.py index c7ee114fe0838c..1e2d572b1cbb81 100644 --- a/Lib/test/test_interpreters/test_api.py +++ b/Lib/test/test_interpreters/test_api.py @@ -647,59 +647,6 @@ def test_created_with_capi(self): self.interp_exists(interpid)) - def test_remaining_threads(self): - r_interp, w_interp = self.pipe() - - FINISHED = b'F' - - # It's unlikely, but technically speaking, it's possible - # that the thread could've finished before interp.close() is - # reached, so this test might not properly exercise the case. - # However, it's quite unlikely and I'm too lazy to deal with it. - interp = interpreters.create() - interp.exec(f"""if True: - import os - import threading - import time - - def task(): - time.sleep(1) - os.write({w_interp}, {FINISHED!r}) - - threads = [threading.Thread(target=task) for _ in range(3)] - for t in threads: - t.start() - """) - interp.close() - - self.assertEqual(os.read(r_interp, 1), FINISHED) - - def test_remaining_daemon_threads(self): - 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(100) - - threads = [threading.Thread(target=task, daemon=True) for _ in range(3)] - for t in threads: - t.start() - """) - _interpreters.destroy(interp) - - class TestInterpreterPrepareMain(TestBase): def test_empty(self): @@ -808,10 +755,7 @@ def script(): spam.eggs() interp = interpreters.create() - try: - interp.exec(script) - finally: - interp.close() + interp.exec(script) """) stdout, stderr = self.assert_python_failure(scriptfile) @@ -820,7 +764,7 @@ def script(): # File "{interpreters.__file__}", line 179, in exec self.assertEqual(stderr, dedent(f"""\ Traceback (most recent call last): - File "{scriptfile}", line 10, in + File "{scriptfile}", line 9, in interp.exec(script) ~~~~~~~~~~~^^^^^^^^ {interpmod_line.strip()} diff --git a/Lib/test/test_interpreters/test_lifecycle.py b/Lib/test/test_interpreters/test_lifecycle.py index 3f9ed1fb501522..ac24f6568acd95 100644 --- a/Lib/test/test_interpreters/test_lifecycle.py +++ b/Lib/test/test_interpreters/test_lifecycle.py @@ -132,7 +132,6 @@ def test_sys_path_0(self): 'sub': sys.path[0], }}, indent=4), flush=True) """) - interp.close() ''' # / # pkg/ @@ -173,10 +172,7 @@ def test_gh_109793(self): argv = [sys.executable, '-c', '''if True: from test.support import interpreters interp = interpreters.create() - try: - raise Exception - finally: - interp.close() + raise Exception '''] proc = subprocess.run(argv, capture_output=True, text=True) self.assertIn('Traceback', proc.stderr) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 127ef658673612..b6e2d419019aa1 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1689,7 +1689,10 @@ def f(): _testcapi.run_in_subinterp(%r) """ % (subinterp_code,) - assert_python_ok("-c", script) + with test.support.SuppressCrashReport(): + rc, out, err = assert_python_failure("-c", script) + self.assertIn("Fatal Python error: Py_EndInterpreter: " + "not the last thread", err.decode()) def _check_allowed(self, before_start='', *, allowed=True, diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-01-08-12-52-47.gh-issue-128640.9nbh9z.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-01-08-12-52-47.gh-issue-128640.9nbh9z.rst deleted file mode 100644 index 040c6d56c47244..00000000000000 --- a/Misc/NEWS.d/next/Core_and_Builtins/2025-01-08-12-52-47.gh-issue-128640.9nbh9z.rst +++ /dev/null @@ -1 +0,0 @@ -Fix a crash when using threads inside of a subinterpreter. diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 0a19d1126e246e..8e0e330f6605c9 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -1395,12 +1395,9 @@ static int test_audit_subinterpreter(void) PySys_AddAuditHook(_audit_subinterpreter_hook, NULL); _testembed_initialize(); - PyThreadState *tstate = PyThreadState_Get(); - for (int i = 0; i < 3; ++i) - { - Py_EndInterpreter(Py_NewInterpreter()); - PyThreadState_Swap(tstate); - } + Py_NewInterpreter(); + Py_NewInterpreter(); + Py_NewInterpreter(); Py_Finalize(); diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index b6bc2ea5211460..8394245d373030 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1992,7 +1992,6 @@ resolve_final_tstate(_PyRuntimeState *runtime) } else { /* Fall back to the current tstate. It's better than nothing. */ - // XXX No it's not main_tstate = tstate; } } @@ -2038,16 +2037,6 @@ _Py_Finalize(_PyRuntimeState *runtime) _PyAtExit_Call(tstate->interp); - /* Clean up any lingering subinterpreters. - - Two preconditions need to be met here: - - - This has to happen before _PyRuntimeState_SetFinalizing is - called, or else threads might get prematurely blocked. - - The world must not be stopped, as finalizers can run. - */ - finalize_subinterpreters(); - assert(_PyThreadState_GET() == tstate); /* Copy the core config, PyInterpreterState_Delete() free @@ -2135,6 +2124,9 @@ _Py_Finalize(_PyRuntimeState *runtime) _PyImport_FiniExternal(tstate->interp); finalize_modules(tstate); + /* Clean up any lingering subinterpreters. */ + finalize_subinterpreters(); + /* Print debug stats if any */ _PyEval_Fini(); @@ -2418,8 +2410,9 @@ Py_NewInterpreter(void) return tstate; } -/* Delete an interpreter. This requires that the given thread state - is current, and that the thread has no remaining frames. +/* Delete an interpreter and its last thread. This requires that the + given thread state is current, that the thread has no remaining + frames, and that it is its interpreter's only remaining thread. It is a fatal error to violate these constraints. (Py_FinalizeEx() doesn't have these constraints -- it zaps @@ -2449,15 +2442,14 @@ Py_EndInterpreter(PyThreadState *tstate) _Py_FinishPendingCalls(tstate); _PyAtExit_Call(tstate->interp); - _PyRuntimeState *runtime = interp->runtime; - _PyEval_StopTheWorldAll(runtime); - PyThreadState *list = _PyThreadState_RemoveExcept(tstate); + + if (tstate != interp->threads.head || tstate->next != NULL) { + Py_FatalError("not the last thread"); + } /* Remaining daemon threads will automatically exit when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyInterpreterState_SetFinalizing(interp, tstate); - _PyEval_StartTheWorldAll(runtime); - _PyThreadState_DeleteList(list, /*is_after_fork=*/0); // XXX Call something like _PyImport_Disable() here? @@ -2488,8 +2480,6 @@ finalize_subinterpreters(void) PyInterpreterState *main_interp = _PyInterpreterState_Main(); assert(final_tstate->interp == main_interp); _PyRuntimeState *runtime = main_interp->runtime; - assert(!runtime->stoptheworld.world_stopped); - assert(_PyRuntimeState_GetFinalizing(runtime) == NULL); struct pyinterpreters *interpreters = &runtime->interpreters; /* Get the first interpreter in the list. */ @@ -2518,17 +2508,27 @@ finalize_subinterpreters(void) /* Clean up all remaining subinterpreters. */ while (interp != NULL) { - /* Make a tstate for finalization. */ - PyThreadState *tstate = _PyThreadState_NewBound(interp, _PyThreadState_WHENCE_FINI); - if (tstate == NULL) { - // XXX Some graceful way to always get a thread state? - Py_FatalError("thread state allocation failed"); + assert(!_PyInterpreterState_IsRunningMain(interp)); + + /* Find the tstate to use for fini. We assume the interpreter + will have at most one tstate at this point. */ + PyThreadState *tstate = interp->threads.head; + if (tstate != NULL) { + /* Ideally we would be able to use tstate as-is, and rely + on it being in a ready state: no exception set, not + running anything (tstate->current_frame), matching the + current thread ID (tstate->thread_id). To play it safe, + we always delete it and use a fresh tstate instead. */ + assert(tstate != final_tstate); + _PyThreadState_Attach(tstate); + PyThreadState_Clear(tstate); + _PyThreadState_Detach(tstate); + PyThreadState_Delete(tstate); } - - /* Enter the subinterpreter. */ - _PyThreadState_Attach(tstate); + tstate = _PyThreadState_NewBound(interp, _PyThreadState_WHENCE_FINI); /* Destroy the subinterpreter. */ + _PyThreadState_Attach(tstate); Py_EndInterpreter(tstate); assert(_PyThreadState_GET() == NULL);