From c2ebedbaf05bb4065f962ac14758ccfdebf8a0bf Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 8 Mar 2024 22:14:48 +0000 Subject: [PATCH 1/3] gh-116522: Stop the world before fork() and during shutdown This changes the free-threaded build to perform a stop-the-world pause before deleting other thread states when forking and during shutdown. This fixes some crashes when using multiprocessing and during shutdown when running with `PYTHON_GIL=0`. This also changes `PyOS_BeforeFork` to acquire the runtime lock (i.e., `HEAD_LOCK(&_PyRuntime)`) before forking to ensure that data protected by the runtime lock (and not just the GIL or stop-the-world) is in a consistent state before forking. --- Modules/posixmodule.c | 12 +++++++++--- Python/pylifecycle.c | 3 +++ Python/pystate.c | 6 ++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 940a9cc8955e11..03dd0bee6ccf07 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -613,11 +613,16 @@ PyOS_BeforeFork(void) run_at_forkers(interp->before_forkers, 1); _PyImport_AcquireLock(interp); + _PyEval_StopTheWorldAll(&_PyRuntime); + HEAD_LOCK(&_PyRuntime); } void PyOS_AfterFork_Parent(void) { + HEAD_UNLOCK(&_PyRuntime); + _PyEval_StartTheWorldAll(&_PyRuntime); + PyInterpreterState *interp = _PyInterpreterState_GET(); if (_PyImport_ReleaseLock(interp) <= 0) { Py_FatalError("failed releasing import lock after fork"); @@ -632,6 +637,7 @@ PyOS_AfterFork_Child(void) PyStatus status; _PyRuntimeState *runtime = &_PyRuntime; + // re-creates runtime->interpreters.mutex (HEAD_UNLOCK) status = _PyRuntimeState_ReInitThreads(runtime); if (_PyStatus_EXCEPTION(status)) { goto fatal_error; @@ -7858,9 +7864,9 @@ os_fork1_impl(PyObject *module) /* child: this clobbers and resets the import lock. */ PyOS_AfterFork_Child(); } else { - warn_about_fork_with_threads("fork1"); /* parent: release the import lock. */ PyOS_AfterFork_Parent(); + warn_about_fork_with_threads("fork1"); } if (pid == -1) { errno = saved_errno; @@ -7906,9 +7912,9 @@ os_fork_impl(PyObject *module) /* child: this clobbers and resets the import lock. */ PyOS_AfterFork_Child(); } else { - warn_about_fork_with_threads("fork"); /* parent: release the import lock. */ PyOS_AfterFork_Parent(); + warn_about_fork_with_threads("fork"); } if (pid == -1) { errno = saved_errno; @@ -8737,9 +8743,9 @@ os_forkpty_impl(PyObject *module) /* child: this clobbers and resets the import lock. */ PyOS_AfterFork_Child(); } else { - warn_about_fork_with_threads("forkpty"); /* parent: release the import lock. */ PyOS_AfterFork_Parent(); + warn_about_fork_with_threads("forkpty"); } if (pid == -1) { return posix_error(); diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 3a2c0a450ac9d9..a936aa92b8e694 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1918,6 +1918,9 @@ Py_FinalizeEx(void) runtime->initialized = 0; runtime->core_initialized = 0; + /* Ensure that remaining threads are detached */ + _PyEval_StopTheWorldAll(runtime); + // XXX Call something like _PyImport_Disable() here? /* Destroy the state of all threads of the interpreter, except of the diff --git a/Python/pystate.c b/Python/pystate.c index 1418d034ca2fe9..8087f16991c377 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1715,6 +1715,10 @@ _PyThreadState_DeleteExcept(PyThreadState *tstate) PyInterpreterState *interp = tstate->interp; _PyRuntimeState *runtime = interp->runtime; +#ifdef Py_GIL_DISABLED + assert(runtime->stoptheworld.world_stopped); +#endif + HEAD_LOCK(runtime); /* Remove all thread states, except tstate, from the linked list of thread states. This will allow calling PyThreadState_Clear() @@ -1733,6 +1737,8 @@ _PyThreadState_DeleteExcept(PyThreadState *tstate) interp->threads.head = tstate; HEAD_UNLOCK(runtime); + _PyEval_StartTheWorldAll(runtime); + /* Clear and deallocate all stale thread states. Even if this executes Python code, we should be safe since it executes in the current thread, not one of the stale threads. */ From b13d1e5de8d5d0423cbd07ca89a8c251b788099d Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 12 Mar 2024 20:36:44 +0000 Subject: [PATCH 2/3] Move stop-the-world call before setting finalized --- Python/pylifecycle.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index a936aa92b8e694..bc76822e72c54a 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1911,6 +1911,9 @@ Py_FinalizeEx(void) int malloc_stats = tstate->interp->config.malloc_stats; #endif + /* Ensure that remaining threads are detached */ + _PyEval_StopTheWorldAll(runtime); + /* Remaining daemon threads will automatically exit when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyInterpreterState_SetFinalizing(tstate->interp, tstate); @@ -1918,9 +1921,6 @@ Py_FinalizeEx(void) runtime->initialized = 0; runtime->core_initialized = 0; - /* Ensure that remaining threads are detached */ - _PyEval_StopTheWorldAll(runtime); - // XXX Call something like _PyImport_Disable() here? /* Destroy the state of all threads of the interpreter, except of the From 4772a69439a484bb1c6c2f3a485c57427839aeb1 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 20 Mar 2024 14:54:48 +0000 Subject: [PATCH 3/3] Add comment and assertion to warn_about_fork_with_threads --- Modules/posixmodule.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 03dd0bee6ccf07..d68e00dde9e090 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -7737,10 +7737,15 @@ os_register_at_fork_impl(PyObject *module, PyObject *before, // running in the process. Best effort, silent if unable to count threads. // Constraint: Quick. Never overcounts. Never leaves an error set. // -// This code might do an import, thus acquiring the import lock, which -// PyOS_BeforeFork() also does. As this should only be called from -// the parent process, it is in the same thread so that works. -static void warn_about_fork_with_threads(const char* name) { +// This should only be called from the parent process after +// PyOS_AfterFork_Parent(). +static void +warn_about_fork_with_threads(const char* name) +{ + // It's not safe to issue the warning while the world is stopped, because + // other threads might be holding locks that we need, which would deadlock. + assert(!_PyRuntime.stoptheworld.world_stopped); + // TODO: Consider making an `os` module API to return the current number // of threads in the process. That'd presumably use this platform code but // raise an error rather than using the inaccurate fallback. @@ -7866,6 +7871,7 @@ os_fork1_impl(PyObject *module) } else { /* parent: release the import lock. */ PyOS_AfterFork_Parent(); + // After PyOS_AfterFork_Parent() starts the world to avoid deadlock. warn_about_fork_with_threads("fork1"); } if (pid == -1) { @@ -7914,6 +7920,7 @@ os_fork_impl(PyObject *module) } else { /* parent: release the import lock. */ PyOS_AfterFork_Parent(); + // After PyOS_AfterFork_Parent() starts the world to avoid deadlock. warn_about_fork_with_threads("fork"); } if (pid == -1) { @@ -8745,6 +8752,7 @@ os_forkpty_impl(PyObject *module) } else { /* parent: release the import lock. */ PyOS_AfterFork_Parent(); + // After PyOS_AfterFork_Parent() starts the world to avoid deadlock. warn_about_fork_with_threads("forkpty"); } if (pid == -1) {