From 2ccb6e198bf85e6aaaf469ebcae43f3df4ff4af7 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 12 Mar 2024 20:14:57 +0000 Subject: [PATCH 1/3] gh-113964: Don't prevent new threads until all non-daemon threads exit Starting in Python 3.12, we started preventing fork() and starting new threads during interpreter finalization (shutdown). This has led to a number of regressions and flaky tests. We should not prevent starting new threads (or fork()) until all non-daemon threads exit and finalization starts in earnest. This changes the checks to use `_PyInterpreterState_GetFinalizing(interp)`, which is set immediately before terminating non-daemon threads. --- Lib/test/test_threading.py | 24 +++++++++++++++++++ ...-03-12-20-31-57.gh-issue-113964.bJppzg.rst | 2 ++ Modules/_posixsubprocess.c | 4 +++- Modules/_threadmodule.c | 2 +- Modules/posixmodule.c | 6 ++--- Objects/unicodeobject.c | 2 +- 6 files changed, 34 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-03-12-20-31-57.gh-issue-113964.bJppzg.rst diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 3b5c37c948c8c34..65ecf7cf2c00e1f 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1340,6 +1340,30 @@ def main(): rc, out, err = assert_python_ok('-c', script) self.assertFalse(err) + def test_thread_from_thread(self): + script = """if True: + import threading + import time + + def thread2(): + time.sleep(0.05) + print("OK") + + def thread1(): + time.sleep(0.05) + t2 = threading.Thread(target=thread2) + t2.start() + + t = threading.Thread(target=thread1) + t.start() + # do not join() -- the interpreter waits for non-daemon threads to + # finish. + """ + rc, out, err = assert_python_ok('-c', script) + self.assertEqual(err, b"") + self.assertEqual(out, b"OK\n") + self.assertEqual(rc, 0) + @skip_unless_reliable_fork def test_reinit_tls_after_fork(self): # Issue #13817: fork() would deadlock in a multithreaded program with diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-03-12-20-31-57.gh-issue-113964.bJppzg.rst b/Misc/NEWS.d/next/Core and Builtins/2024-03-12-20-31-57.gh-issue-113964.bJppzg.rst new file mode 100644 index 000000000000000..ab370d4aa1baeea --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-03-12-20-31-57.gh-issue-113964.bJppzg.rst @@ -0,0 +1,2 @@ +Starting new threads and process creation through :func:`os.fork` are now +only prevented once all non-daemon threads exit. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index bcbbe70680b8e76..b160cd78177a175 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -1031,7 +1031,9 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args, Py_ssize_t fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep); PyInterpreterState *interp = _PyInterpreterState_GET(); - if ((preexec_fn != Py_None) && interp->finalizing) { + if ((preexec_fn != Py_None) && + _PyInterpreterState_GetFinalizing(interp) != NULL) + { PyErr_SetString(PyExc_PythonFinalizationError, "preexec_fn not supported at interpreter shutdown"); return NULL; diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index cc5396a035018f2..d7eaf18b0fcb807 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1395,7 +1395,7 @@ do_start_new_thread(thread_module_state* state, "thread is not supported for isolated subinterpreters"); return -1; } - if (interp->finalizing) { + if (_PyInterpreterState_GetFinalizing(interp) != NULL) { PyErr_SetString(PyExc_PythonFinalizationError, "can't create new thread at interpreter shutdown"); return -1; diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 19e925730a5110a..dca6b1fe23c4f5a 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -7842,7 +7842,7 @@ os_fork1_impl(PyObject *module) pid_t pid; PyInterpreterState *interp = _PyInterpreterState_GET(); - if (interp->finalizing) { + if (_PyInterpreterState_GetFinalizing(interp) != NULL) { PyErr_SetString(PyExc_PythonFinalizationError, "can't fork at interpreter shutdown"); return NULL; @@ -7886,7 +7886,7 @@ os_fork_impl(PyObject *module) { pid_t pid; PyInterpreterState *interp = _PyInterpreterState_GET(); - if (interp->finalizing) { + if (_PyInterpreterState_GetFinalizing(interp) != NULL) { PyErr_SetString(PyExc_PythonFinalizationError, "can't fork at interpreter shutdown"); return NULL; @@ -8719,7 +8719,7 @@ os_forkpty_impl(PyObject *module) pid_t pid; PyInterpreterState *interp = _PyInterpreterState_GET(); - if (interp->finalizing) { + if (_PyInterpreterState_GetFinalizing(interp) != NULL) { PyErr_SetString(PyExc_PythonFinalizationError, "can't fork at interpreter shutdown"); return NULL; diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c index 0a569a950e88e29..b785cd9624911b2 100644 --- a/Objects/unicodeobject.c +++ b/Objects/unicodeobject.c @@ -505,7 +505,7 @@ unicode_check_encoding_errors(const char *encoding, const char *errors) /* Disable checks during Python finalization. For example, it allows to call _PyObject_Dump() during finalization for debugging purpose. */ - if (interp->finalizing) { + if (_PyInterpreterState_GetFinalizing(interp) != NULL) { return 0; } From 6b7d01eb47131f26a1c4acb7faac5a4f35674e4d Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 12 Mar 2024 20:49:59 +0000 Subject: [PATCH 2/3] Update tests --- Lib/test/test_os.py | 17 +++++++++-------- Lib/test/test_subprocess.py | 13 +++++++------ Lib/test/test_threading.py | 14 +++++++------- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index fc886f967c11bf7..a9e2088d1de7f91 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -5283,20 +5283,21 @@ def test_fork_warns_when_non_python_thread_exists(self): self.assertEqual(err.decode("utf-8"), "") self.assertEqual(out.decode("utf-8"), "") - def test_fork_at_exit(self): + def test_fork_at_finalization(self): code = """if 1: import atexit import os - def exit_handler(): - pid = os.fork() - if pid != 0: - print("shouldn't be printed") - - atexit.register(exit_handler) + class AtFinalization: + def __del__(self): + print("OK") + pid = os.fork() + if pid != 0: + print("shouldn't be printed") + at_finalization = AtFinalization() """ _, out, err = assert_python_ok("-c", code) - self.assertEqual(b"", out) + self.assertEqual(b"OK\n", out) self.assertIn(b"can't fork at interpreter shutdown", err) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index c44a778d5bbefe5..ff4f3988aabea97 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3382,14 +3382,15 @@ def test_preexec_at_exit(self): def dummy(): pass - def exit_handler(): - subprocess.Popen({ZERO_RETURN_CMD}, preexec_fn=dummy) - print("shouldn't be printed") - - atexit.register(exit_handler) + class AtFinalization: + def __del__(self): + print("OK") + subprocess.Popen({ZERO_RETURN_CMD}, preexec_fn=dummy) + print("shouldn't be printed") + at_finalization = AtFinalization() """ _, out, err = assert_python_ok("-c", code) - self.assertEqual(out, b'') + self.assertEqual(out, b'OK\n') self.assertIn(b"preexec_fn not supported at interpreter shutdown", err) @unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"), diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 65ecf7cf2c00e1f..d27ed0a15e9099f 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1202,21 +1202,21 @@ def import_threading(): self.assertEqual(out, b'') self.assertEqual(err, b'') - def test_start_new_thread_at_exit(self): + def test_start_new_thread_at_finalization(self): code = """if 1: - import atexit import _thread def f(): print("shouldn't be printed") - def exit_handler(): - _thread.start_new_thread(f, ()) - - atexit.register(exit_handler) + class AtFinalization: + def __del__(self): + print("OK") + _thread.start_new_thread(f, ()) + at_finalization = AtFinalization() """ _, out, err = assert_python_ok("-c", code) - self.assertEqual(out, b'') + self.assertEqual(out, b'OK\n') self.assertIn(b"can't create new thread at interpreter shutdown", err) class ThreadJoinOnShutdown(BaseTestCase): From 27e4bc58c909de70cdbe7e2e556e253a5a44ec84 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 13 Mar 2024 14:49:21 +0000 Subject: [PATCH 3/3] Strip output --- Lib/test/test_subprocess.py | 2 +- Lib/test/test_threading.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index ff4f3988aabea97..79f9b22ffb1a987 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3390,7 +3390,7 @@ def __del__(self): at_finalization = AtFinalization() """ _, out, err = assert_python_ok("-c", code) - self.assertEqual(out, b'OK\n') + self.assertEqual(out.strip(), b"OK") self.assertIn(b"preexec_fn not supported at interpreter shutdown", err) @unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"), diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index d27ed0a15e9099f..f34a58c7a8192ff 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1216,7 +1216,7 @@ def __del__(self): at_finalization = AtFinalization() """ _, out, err = assert_python_ok("-c", code) - self.assertEqual(out, b'OK\n') + self.assertEqual(out.strip(), b"OK") self.assertIn(b"can't create new thread at interpreter shutdown", err) class ThreadJoinOnShutdown(BaseTestCase): @@ -1361,7 +1361,7 @@ def thread1(): """ rc, out, err = assert_python_ok('-c', script) self.assertEqual(err, b"") - self.assertEqual(out, b"OK\n") + self.assertEqual(out.strip(), b"OK") self.assertEqual(rc, 0) @skip_unless_reliable_fork