From b14aa73da89cf07c0e841b023816211f9fea5aef Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 21 May 2025 20:17:39 -0400 Subject: [PATCH 1/6] Reapply "gh-128639: Don't assume one thread in subinterpreter finalization (gh-128640)" (gh-134256) This reverts commit 27bd08273ce822a4dbe0e73cca47441e99fd6f0d. --- 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, 99 insertions(+), 38 deletions(-) create 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 1e2d572b1cbb81..c7ee114fe0838c 100644 --- a/Lib/test/test_interpreters/test_api.py +++ b/Lib/test/test_interpreters/test_api.py @@ -647,6 +647,59 @@ 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): @@ -755,7 +808,10 @@ def script(): spam.eggs() interp = interpreters.create() - interp.exec(script) + try: + interp.exec(script) + finally: + interp.close() """) stdout, stderr = self.assert_python_failure(scriptfile) @@ -764,7 +820,7 @@ def script(): # File "{interpreters.__file__}", line 179, in exec self.assertEqual(stderr, dedent(f"""\ Traceback (most recent call last): - File "{scriptfile}", line 9, in + File "{scriptfile}", line 10, 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 ac24f6568acd95..3f9ed1fb501522 100644 --- a/Lib/test/test_interpreters/test_lifecycle.py +++ b/Lib/test/test_interpreters/test_lifecycle.py @@ -132,6 +132,7 @@ def test_sys_path_0(self): 'sub': sys.path[0], }}, indent=4), flush=True) """) + interp.close() ''' # / # pkg/ @@ -172,7 +173,10 @@ def test_gh_109793(self): argv = [sys.executable, '-c', '''if True: from test.support import interpreters interp = interpreters.create() - raise Exception + try: + raise Exception + finally: + interp.close() '''] 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 0e51e7fc8c5a76..c84034ef294a95 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1718,10 +1718,7 @@ def f(): _testcapi.run_in_subinterp(%r) """ % (subinterp_code,) - 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()) + assert_python_ok("-c", script) 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 new file mode 100644 index 00000000000000..040c6d56c47244 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-01-08-12-52-47.gh-issue-128640.9nbh9z.rst @@ -0,0 +1 @@ +Fix a crash when using threads inside of a subinterpreter. diff --git a/Programs/_testembed.c b/Programs/_testembed.c index 577da65c7cdafa..79c3a3f6facf92 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -1396,9 +1396,12 @@ static int test_audit_subinterpreter(void) PySys_AddAuditHook(_audit_subinterpreter_hook, NULL); _testembed_initialize(); - Py_NewInterpreter(); - Py_NewInterpreter(); - Py_NewInterpreter(); + PyThreadState *tstate = PyThreadState_Get(); + for (int i = 0; i < 3; ++i) + { + Py_EndInterpreter(Py_NewInterpreter()); + PyThreadState_Swap(tstate); + } Py_Finalize(); diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 8394245d373030..b6bc2ea5211460 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1992,6 +1992,7 @@ 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; } } @@ -2037,6 +2038,16 @@ _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 @@ -2124,9 +2135,6 @@ _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(); @@ -2410,9 +2418,8 @@ Py_NewInterpreter(void) return tstate; } -/* 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. +/* Delete an interpreter. This requires that the given thread state + is current, and that the thread has no remaining frames. It is a fatal error to violate these constraints. (Py_FinalizeEx() doesn't have these constraints -- it zaps @@ -2442,14 +2449,15 @@ Py_EndInterpreter(PyThreadState *tstate) _Py_FinishPendingCalls(tstate); _PyAtExit_Call(tstate->interp); - - if (tstate != interp->threads.head || tstate->next != NULL) { - Py_FatalError("not the last thread"); - } + _PyRuntimeState *runtime = interp->runtime; + _PyEval_StopTheWorldAll(runtime); + PyThreadState *list = _PyThreadState_RemoveExcept(tstate); /* 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? @@ -2480,6 +2488,8 @@ 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. */ @@ -2508,27 +2518,17 @@ finalize_subinterpreters(void) /* Clean up all remaining subinterpreters. */ while (interp != NULL) { - 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); + /* 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"); } - tstate = _PyThreadState_NewBound(interp, _PyThreadState_WHENCE_FINI); - /* Destroy the subinterpreter. */ + /* Enter the subinterpreter. */ _PyThreadState_Attach(tstate); + + /* Destroy the subinterpreter. */ Py_EndInterpreter(tstate); assert(_PyThreadState_GET() == NULL); From 91d94d9ee736795c7cc2d8ab3412be7ba9f8a29b Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Fri, 23 May 2025 20:19:31 -0400 Subject: [PATCH 2/6] Fix daemon thread shutdown. --- Lib/test/test_interpreters/test_api.py | 2 +- Python/pylifecycle.c | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_interpreters/test_api.py b/Lib/test/test_interpreters/test_api.py index c7ee114fe0838c..0d8b3bebfe925a 100644 --- a/Lib/test/test_interpreters/test_api.py +++ b/Lib/test/test_interpreters/test_api.py @@ -691,7 +691,7 @@ def test_remaining_daemon_threads(self): import time def task(): - time.sleep(100) + time.sleep(3) threads = [threading.Thread(target=task, daemon=True) for _ in range(3)] for t in threads: diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index b6bc2ea5211460..175691a48356c1 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2449,14 +2449,17 @@ Py_EndInterpreter(PyThreadState *tstate) _Py_FinishPendingCalls(tstate); _PyAtExit_Call(tstate->interp); - _PyRuntimeState *runtime = interp->runtime; - _PyEval_StopTheWorldAll(runtime); - PyThreadState *list = _PyThreadState_RemoveExcept(tstate); - + _PyEval_StopTheWorld(interp); /* Remaining daemon threads will automatically exit when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyInterpreterState_SetFinalizing(interp, tstate); - _PyEval_StartTheWorldAll(runtime); + + PyThreadState *list = _PyThreadState_RemoveExcept(tstate); + for (PyThreadState *p = list; p != NULL; p = p->next) { + _PyThreadState_SetShuttingDown(p); + } + + _PyEval_StartTheWorld(interp); _PyThreadState_DeleteList(list, /*is_after_fork=*/0); // XXX Call something like _PyImport_Disable() here? From a5693559bdf322a2b62058b4aad1b3861fdaaf43 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Fri, 23 May 2025 20:35:25 -0400 Subject: [PATCH 3/6] Stop the runtime instead of the interpreter. --- Python/pylifecycle.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 175691a48356c1..a8b6472b48d6ad 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2449,7 +2449,8 @@ Py_EndInterpreter(PyThreadState *tstate) _Py_FinishPendingCalls(tstate); _PyAtExit_Call(tstate->interp); - _PyEval_StopTheWorld(interp); + _PyRuntimeState *runtime = interp->runtime; + _PyEval_StopTheWorldAll(runtime); /* Remaining daemon threads will automatically exit when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyInterpreterState_SetFinalizing(interp, tstate); @@ -2459,7 +2460,7 @@ Py_EndInterpreter(PyThreadState *tstate) _PyThreadState_SetShuttingDown(p); } - _PyEval_StartTheWorld(interp); + _PyEval_StartTheWorldAll(runtime); _PyThreadState_DeleteList(list, /*is_after_fork=*/0); // XXX Call something like _PyImport_Disable() here? From f85a291b5d70e8b25ac89342aff708c87fc95ba0 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 25 May 2025 11:59:06 -0400 Subject: [PATCH 4/6] Run the daemon thread test in a subprocess. --- Lib/test/test_interpreters/test_api.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Lib/test/test_interpreters/test_api.py b/Lib/test/test_interpreters/test_api.py index 0d8b3bebfe925a..2519bb8ec618eb 100644 --- a/Lib/test/test_interpreters/test_api.py +++ b/Lib/test/test_interpreters/test_api.py @@ -7,6 +7,7 @@ from test import support from test.support import import_helper +from test.support.script_helper import assert_python_ok # Raise SkipTest if subinterpreters not supported. _interpreters = import_helper.import_module('_interpreters') from test.support import Py_GIL_DISABLED @@ -675,6 +676,13 @@ def task(): self.assertEqual(os.read(r_interp, 1), FINISHED) def test_remaining_daemon_threads(self): + # Daemon threads leak reference by nature, because they hang threads + # without allowing them to do cleanup (i.e., release refs). + # To prevent that from messing up the refleak hunter and whatnot, we + # run this in a subprocess. + code = '''if True: + import _interpreters + import types interp = _interpreters.create( types.SimpleNamespace( use_main_obmalloc=False, @@ -698,6 +706,8 @@ def task(): t.start() """) _interpreters.destroy(interp) + ''' + assert_python_ok('-c', code) class TestInterpreterPrepareMain(TestBase): From d287d54f0f7327aa30a2a75142298bf3a14e57e5 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 27 May 2025 11:12:17 -0400 Subject: [PATCH 5/6] Update Lib/test/test_interpreters/test_api.py Co-authored-by: Eric Snow --- Lib/test/test_interpreters/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_interpreters/test_api.py b/Lib/test/test_interpreters/test_api.py index 2519bb8ec618eb..c70baf3bb020b9 100644 --- a/Lib/test/test_interpreters/test_api.py +++ b/Lib/test/test_interpreters/test_api.py @@ -656,7 +656,7 @@ def test_remaining_threads(self): # 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. + # However, it's quite unlikely and probably not worth bothering about. interp = interpreters.create() interp.exec(f"""if True: import os From 62bd6532cfc7e7fc60e7a8150f116caada3b32f4 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 8 Jun 2025 17:07:53 -0400 Subject: [PATCH 6/6] Remove broken assertion. --- Objects/codeobject.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Objects/codeobject.c b/Objects/codeobject.c index ee869d991d93cd..e7dc8750076783 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -1979,7 +1979,9 @@ _PyCode_CheckNoExternalState(PyCodeObject *co, _PyCode_var_counts_t *counts, const char **p_errmsg) { const char *errmsg = NULL; - assert(counts->locals.hidden.total == 0); + // Why is it an assumption that there can't be any hidden + // locals? + //assert(counts->locals.hidden.total == 0); if (counts->numfree > 0) { // It's a closure. errmsg = "closures not supported"; }