From 025cc1723113f1ad0c1d100bd8d9a8d4ed26e19c Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 8 Jan 2025 12:26:02 -0500 Subject: [PATCH 01/15] Fix subinterpreter cleanup. --- Python/pylifecycle.c | 52 +++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 06418123d6dd9b..02588e6426beba 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2054,12 +2054,23 @@ _Py_Finalize(_PyRuntimeState *runtime) int malloc_stats = tstate->interp->config.malloc_stats; #endif + /* 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(); + /* Ensure that remaining threads are detached */ _PyEval_StopTheWorldAll(runtime); /* Remaining daemon threads will be trapped in PyThread_hang_thread when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyInterpreterState_SetFinalizing(tstate->interp, tstate); + _PyRuntimeState_SetFinalizing(runtime, tstate); runtime->initialized = 0; runtime->core_initialized = 0; @@ -2121,9 +2132,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(); @@ -2438,14 +2446,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); // XXX Call something like _PyImport_Disable() here? @@ -2476,6 +2485,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. */ @@ -2504,27 +2515,18 @@ 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 eea3ba95fc1bb34cf7df0164928b9d591efa87a1 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 8 Jan 2025 12:48:27 -0500 Subject: [PATCH 02/15] Add and fix tests. --- Lib/test/test_interpreters/test_api.py | 56 +++++++++++++++++++- Lib/test/test_interpreters/test_lifecycle.py | 6 ++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_interpreters/test_api.py b/Lib/test/test_interpreters/test_api.py index 01856d9bf67657..daa7916a27c266 100644 --- a/Lib/test/test_interpreters/test_api.py +++ b/Lib/test/test_interpreters/test_api.py @@ -647,6 +647,55 @@ def test_created_with_capi(self): self.interp_exists(interpid)) + def test_remaining_threads(self): + r_interp, w_interp = self.pipe() + + FINISHED = b'F' + + 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): @@ -756,7 +805,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) @@ -765,7 +817,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) From 149e0cf43b76c20eae699d3d0532c98c2b87f57b Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 8 Jan 2025 12:52:53 -0500 Subject: [PATCH 03/15] Add blurb --- .../2025-01-08-12-52-47.gh-issue-128640.9nbh9z.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-01-08-12-52-47.gh-issue-128640.9nbh9z.rst 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. From 1761c3759a64d0597dfa5c961b175851afe8e0e6 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 8 Jan 2025 13:43:42 -0500 Subject: [PATCH 04/15] Fix threading tests. --- Lib/test/test_threading.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 3e164a12581dd1..5cd687edcdc7fb 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1595,6 +1595,8 @@ def task(): @cpython_only def test_daemon_threads_fatal_error(self): + # This used to crash, but Py_EndInterpreter() can now + # handle remaining threads. import_module("_testcapi") subinterp_code = f"""if 1: import os @@ -1612,10 +1614,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, From 8febfbfcd7c82e9c2ce1b152d49cd40af6a74587 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 8 Jan 2025 14:37:30 -0500 Subject: [PATCH 05/15] Fix test_embed. --- Programs/_testembed.c | 9 ++++++--- Python/pylifecycle.c | 5 ++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Programs/_testembed.c b/Programs/_testembed.c index d15dd519dbf6af..6a501e0519461e 100644 --- a/Programs/_testembed.c +++ b/Programs/_testembed.c @@ -1424,9 +1424,12 @@ static int test_audit_subinterpreter(void) PySys_AddAuditHook(_audit_subinterpreter_hook, NULL); _testembed_Py_InitializeFromConfig(); - 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 02588e6426beba..5c032fd25fe313 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2414,9 +2414,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 From 5a73ce1d7fe751b62027fc851d44b7db245d83e8 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 8 Jan 2025 14:53:45 -0500 Subject: [PATCH 06/15] Remove buggy test. --- Lib/test/test_threading.py | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 5cd687edcdc7fb..a475850b93e122 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1593,29 +1593,6 @@ def task(): self.assertEqual(os.read(r_interp, 1), FINI) self.assertEqual(os.read(r_interp, 1), DONE) - @cpython_only - def test_daemon_threads_fatal_error(self): - # This used to crash, but Py_EndInterpreter() can now - # handle remaining threads. - import_module("_testcapi") - subinterp_code = f"""if 1: - import os - import threading - import time - - def f(): - # Make sure the daemon thread is still running when - # Py_EndInterpreter is called. - time.sleep({test.support.SHORT_TIMEOUT}) - threading.Thread(target=f, daemon=True).start() - """ - script = r"""if 1: - import _testcapi - - _testcapi.run_in_subinterp(%r) - """ % (subinterp_code,) - assert_python_ok("-c", script) - def _check_allowed(self, before_start='', *, allowed=True, daemon_allowed=True, From a7cc3fe32ba709e2b9d54372fe07c0b34dd0647f Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Wed, 8 Jan 2025 14:54:47 -0500 Subject: [PATCH 07/15] Skip flaky test. --- Lib/test/test_threading.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index a475850b93e122..af931ba2be77ff 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1593,6 +1593,28 @@ def task(): self.assertEqual(os.read(r_interp, 1), FINI) self.assertEqual(os.read(r_interp, 1), DONE) + @cpython_only + @unittest.skipIf(True, "only crashes some of the time now that Py_EndInterpreter() can handle more threads") + def test_daemon_threads_fatal_error(self): + import_module("_testcapi") + subinterp_code = f"""if 1: + import os + import threading + import time + + def f(): + # Make sure the daemon thread is still running when + # Py_EndInterpreter is called. + time.sleep({test.support.SHORT_TIMEOUT}) + threading.Thread(target=f, daemon=True).start() + """ + script = r"""if 1: + import _testcapi + + _testcapi.run_in_subinterp(%r) + """ % (subinterp_code,) + assert_python_ok("-c", script) + def _check_allowed(self, before_start='', *, allowed=True, daemon_allowed=True, From 7c44c0bfb454e03da14c6a6f736a93468c742b62 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 3 Feb 2025 20:58:22 -0500 Subject: [PATCH 08/15] Add an assertion. --- Python/pylifecycle.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 5c032fd25fe313..fd02c2502bce06 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2002,6 +2002,7 @@ resolve_final_tstate(_PyRuntimeState *runtime) /* We might want to warn if main_tstate->current_frame != NULL. */ + assert(main_tstate->interp == main_interp); return main_tstate; } From 6551bfa25ef137e2ed727fca66575d8e16c3a27f Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 4 Feb 2025 20:05:58 -0500 Subject: [PATCH 09/15] Remove stray newline. --- Python/pylifecycle.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index fd02c2502bce06..703f89da18b9fa 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2071,7 +2071,6 @@ _Py_Finalize(_PyRuntimeState *runtime) /* Remaining daemon threads will be trapped in PyThread_hang_thread when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyInterpreterState_SetFinalizing(tstate->interp, tstate); - _PyRuntimeState_SetFinalizing(runtime, tstate); runtime->initialized = 0; runtime->core_initialized = 0; From f19e28d631baf23f5d93aa1678746e3b486c2d6c Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 4 Feb 2025 20:07:52 -0500 Subject: [PATCH 10/15] Add another assertion. --- Python/pylifecycle.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 703f89da18b9fa..dbaeeacf8abf73 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1994,6 +1994,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; } } @@ -2018,6 +2019,8 @@ _Py_Finalize(_PyRuntimeState *runtime) /* Get final thread state pointer. */ PyThreadState *tstate = resolve_final_tstate(runtime); + // We must be in the main interpreter + assert(tstate->interp == &runtime->_main_interpreter); // Block some operations. tstate->interp->finalizing = 1; From 76e4ec991c77cfe744a38de97a52a79cfaea3f9a Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Fri, 28 Mar 2025 16:10:02 -0400 Subject: [PATCH 11/15] Use new is_after_fork parameter --- Python/pylifecycle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 897f8995f6b7ac..2ae535fa5ca086 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2450,7 +2450,7 @@ Py_EndInterpreter(PyThreadState *tstate) when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyInterpreterState_SetFinalizing(interp, tstate); _PyEval_StartTheWorldAll(runtime); - _PyThreadState_DeleteList(list); + _PyThreadState_DeleteList(list, /*is_after_fork=*/0); // XXX Call something like _PyImport_Disable() here? From 1555731dc2ea6acb895f17f4e20d30fb9041dade Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 11:16:47 -0400 Subject: [PATCH 12/15] Apply suggestions from code review Co-authored-by: Eric Snow --- Python/pylifecycle.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 2ae535fa5ca086..fed8126eb66f94 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1991,7 +1991,6 @@ resolve_final_tstate(_PyRuntimeState *runtime) /* We might want to warn if main_tstate->current_frame != NULL. */ - assert(main_tstate->interp == main_interp); return main_tstate; } @@ -2007,8 +2006,6 @@ _Py_Finalize(_PyRuntimeState *runtime) /* Get final thread state pointer. */ PyThreadState *tstate = resolve_final_tstate(runtime); - // We must be in the main interpreter - assert(tstate->interp == &runtime->_main_interpreter); // Block some operations. tstate->interp->finalizing = 1; @@ -2513,8 +2510,7 @@ finalize_subinterpreters(void) while (interp != NULL) { /* Make a tstate for finalization. */ PyThreadState *tstate = _PyThreadState_NewBound(interp, _PyThreadState_WHENCE_FINI); - if (tstate == NULL) - { + if (tstate == NULL) { // XXX Some graceful way to always get a thread state? Py_FatalError("thread state allocation failed"); } From d61099eefe9f524603436b258851633e2baf1b76 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 11:31:40 -0400 Subject: [PATCH 13/15] Move call to finalize_subinterpreters() --- Python/pylifecycle.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index fed8126eb66f94..0ef5269696dec2 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2028,6 +2028,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 @@ -2043,16 +2053,6 @@ _Py_Finalize(_PyRuntimeState *runtime) int malloc_stats = tstate->interp->config.malloc_stats; #endif - /* 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(); - /* Ensure that remaining threads are detached */ _PyEval_StopTheWorldAll(runtime); From 09851dce5af90f4594dd31a55d020a0aaefda99a Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 11:34:10 -0400 Subject: [PATCH 14/15] Unskip the threading test. --- Lib/test/test_threading.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index de331d7eef4b76..e5a4c0825cbb0a 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1594,7 +1594,6 @@ def task(): self.assertEqual(os.read(r_interp, 1), DONE) @cpython_only - @unittest.skipIf(True, "only crashes some of the time now that Py_EndInterpreter() can handle more threads") def test_daemon_threads_fatal_error(self): import_module("_testcapi") subinterp_code = f"""if 1: From b223396f18b28003abefc6491abab5961c56fe6b Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Mon, 19 May 2025 11:36:56 -0400 Subject: [PATCH 15/15] Add a comment for the test. --- Lib/test/test_interpreters/test_api.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Lib/test/test_interpreters/test_api.py b/Lib/test/test_interpreters/test_api.py index daa7916a27c266..5238d7217a94bb 100644 --- a/Lib/test/test_interpreters/test_api.py +++ b/Lib/test/test_interpreters/test_api.py @@ -652,6 +652,10 @@ def test_remaining_threads(self): 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