[Python-checkins] gh-100228: Warn from os.fork() if other threads exist. (#100229)

https://github.com/python/cpython/commit/894f2c3c161933bd820ad322b3b678d89bc... commit: 894f2c3c161933bd820ad322b3b678d89bc2377c branch: main author: Gregory P. Smith <greg@krypto.org> committer: gpshead <greg@krypto.org> date: 2022-12-29T14:41:39-08:00 summary: gh-100228: Warn from os.fork() if other threads exist. (#100229) Not comprehensive, best effort warning. There are cases when threads exist on some platforms that this code cannot detect. macOS when API permissions allow and Linux with a readable /proc procfs present are the currently supported cases where a warning should show up reliably. Starting with a DeprecationWarning for now, it is less disruptive than something like RuntimeWarning and most likely to only be seen in people's CI tests - a good place to start with this messaging. files: A Misc/NEWS.d/next/Library/2022-12-13-17-29-09.gh-issue-100228.bgtzMV.rst M Include/internal/pycore_global_objects_fini_generated.h M Include/internal/pycore_global_strings.h M Include/internal/pycore_runtime_init_generated.h M Include/internal/pycore_unicodeobject_generated.h M Lib/test/fork_wait.py M Lib/test/test_os.py M Lib/test/test_thread.py M Lib/test/test_threading.py M Lib/threading.py M Modules/_testcapimodule.c M Modules/posixmodule.c diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.h index 6aba2f19ebde..a5365d53150b 100644 --- a/Include/internal/pycore_global_objects_fini_generated.h +++ b/Include/internal/pycore_global_objects_fini_generated.h @@ -733,6 +733,7 @@ _PyStaticObjects_CheckRefcnt(PyInterpreterState *interp) { _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(__xor__)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_abc_impl)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_abstract_)); + _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_active)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_annotation)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_anonymous_)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_argtypes_)); @@ -753,6 +754,7 @@ _PyStaticObjects_CheckRefcnt(PyInterpreterState *interp) { _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_initializing)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_is_text_encoding)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_length_)); + _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_limbo)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_lock_unlock_module)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_loop)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_needs_com_addref_)); diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.h index acb9a4fbb92d..3d9e61e50ecc 100644 --- a/Include/internal/pycore_global_strings.h +++ b/Include/internal/pycore_global_strings.h @@ -219,6 +219,7 @@ struct _Py_global_strings { STRUCT_FOR_ID(__xor__) STRUCT_FOR_ID(_abc_impl) STRUCT_FOR_ID(_abstract_) + STRUCT_FOR_ID(_active) STRUCT_FOR_ID(_annotation) STRUCT_FOR_ID(_anonymous_) STRUCT_FOR_ID(_argtypes_) @@ -239,6 +240,7 @@ struct _Py_global_strings { STRUCT_FOR_ID(_initializing) STRUCT_FOR_ID(_is_text_encoding) STRUCT_FOR_ID(_length_) + STRUCT_FOR_ID(_limbo) STRUCT_FOR_ID(_lock_unlock_module) STRUCT_FOR_ID(_loop) STRUCT_FOR_ID(_needs_com_addref_) diff --git a/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.h index 6d1b8702c776..3534b94753e0 100644 --- a/Include/internal/pycore_runtime_init_generated.h +++ b/Include/internal/pycore_runtime_init_generated.h @@ -725,6 +725,7 @@ extern "C" { INIT_ID(__xor__), \ INIT_ID(_abc_impl), \ INIT_ID(_abstract_), \ + INIT_ID(_active), \ INIT_ID(_annotation), \ INIT_ID(_anonymous_), \ INIT_ID(_argtypes_), \ @@ -745,6 +746,7 @@ extern "C" { INIT_ID(_initializing), \ INIT_ID(_is_text_encoding), \ INIT_ID(_length_), \ + INIT_ID(_limbo), \ INIT_ID(_lock_unlock_module), \ INIT_ID(_loop), \ INIT_ID(_needs_com_addref_), \ diff --git a/Include/internal/pycore_unicodeobject_generated.h b/Include/internal/pycore_unicodeobject_generated.h index 7f407c0141b8..02435071bb2c 100644 --- a/Include/internal/pycore_unicodeobject_generated.h +++ b/Include/internal/pycore_unicodeobject_generated.h @@ -344,6 +344,8 @@ _PyUnicode_InitStaticStrings(void) { PyUnicode_InternInPlace(&string); string = &_Py_ID(_abstract_); PyUnicode_InternInPlace(&string); + string = &_Py_ID(_active); + PyUnicode_InternInPlace(&string); string = &_Py_ID(_annotation); PyUnicode_InternInPlace(&string); string = &_Py_ID(_anonymous_); @@ -384,6 +386,8 @@ _PyUnicode_InitStaticStrings(void) { PyUnicode_InternInPlace(&string); string = &_Py_ID(_length_); PyUnicode_InternInPlace(&string); + string = &_Py_ID(_limbo); + PyUnicode_InternInPlace(&string); string = &_Py_ID(_lock_unlock_module); PyUnicode_InternInPlace(&string); string = &_Py_ID(_loop); diff --git a/Lib/test/fork_wait.py b/Lib/test/fork_wait.py index ebd07e612e58..c26c7aaaeb43 100644 --- a/Lib/test/fork_wait.py +++ b/Lib/test/fork_wait.py @@ -13,6 +13,7 @@ import threading from test import support from test.support import threading_helper +import warnings LONGSLEEP = 2 @@ -63,19 +64,17 @@ def test_wait(self): prefork_lives = self.alive.copy() - if sys.platform in ['unixware7']: - cpid = os.fork1() - else: - cpid = os.fork() - - if cpid == 0: - # Child - time.sleep(LONGSLEEP) - n = 0 - for key in self.alive: - if self.alive[key] != prefork_lives[key]: - n += 1 - os._exit(n) - else: - # Parent - self.wait_impl(cpid, exitcode=0) + # Ignore the warning about fork with threads. + with warnings.catch_warnings(category=DeprecationWarning, + action="ignore"): + if (cpid := os.fork()) == 0: + # Child + time.sleep(LONGSLEEP) + n = 0 + for key in self.alive: + if self.alive[key] != prefork_lives[key]: + n += 1 + os._exit(n) + else: + # Parent + self.wait_impl(cpid, exitcode=0) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index e6e25b507de0..58e04dd1348f 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -4577,6 +4577,34 @@ def test_fork(self): assert_python_ok("-c", code) assert_python_ok("-c", code, PYTHONMALLOC="malloc_debug") + @unittest.skipUnless(sys.platform in ("linux", "darwin"), + "Only Linux and macOS detect this today.") + def test_fork_warns_when_non_python_thread_exists(self): + code = """if 1: + import os, threading, warnings + from _testcapi import _spawn_pthread_waiter, _end_spawned_pthread + _spawn_pthread_waiter() + try: + with warnings.catch_warnings(record=True) as ws: + warnings.filterwarnings( + "always", category=DeprecationWarning) + if os.fork() == 0: + assert not ws, f"unexpected warnings in child: {ws}" + os._exit(0) # child + else: + assert ws[0].category == DeprecationWarning, ws[0] + assert 'fork' in str(ws[0].message), ws[0] + # Waiting allows an error in the child to hit stderr. + exitcode = os.wait()[1] + assert exitcode == 0, f"child exited {exitcode}" + assert threading.active_count() == 1, threading.enumerate() + finally: + _end_spawned_pthread() + """ + _, out, err = assert_python_ok("-c", code, PYTHONOPTIMIZE='0') + self.assertEqual(err.decode("utf-8"), "") + self.assertEqual(out.decode("utf-8"), "") + # Only test if the C version is provided, otherwise TestPEP519 already tested # the pure Python implementation. diff --git a/Lib/test/test_thread.py b/Lib/test/test_thread.py index 2ae5e9ccb440..8656fbdd83e9 100644 --- a/Lib/test/test_thread.py +++ b/Lib/test/test_thread.py @@ -5,6 +5,7 @@ from test.support import threading_helper import _thread as thread import time +import warnings import weakref from test import lock_tests @@ -238,11 +239,13 @@ def test_forkinthread(self): def fork_thread(read_fd, write_fd): nonlocal pid - # fork in a thread - pid = os.fork() - if pid: - # parent process - return + # Ignore the warning about fork with threads. + with warnings.catch_warnings(category=DeprecationWarning, + action="ignore"): + # fork in a thread (DANGER, undefined per POSIX) + if (pid := os.fork()): + # parent process + return # child process try: diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 13ba5068ae20..31bf46311a80 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -20,6 +20,7 @@ import signal import textwrap import traceback +import warnings from unittest import mock from test import lock_tests @@ -563,7 +564,7 @@ def test_dummy_thread_after_fork(self): # Issue #14308: a dummy thread in the active list doesn't mess up # the after-fork mechanism. code = """if 1: - import _thread, threading, os, time + import _thread, threading, os, time, warnings def background_thread(evt): # Creates and registers the _DummyThread instance @@ -575,11 +576,16 @@ def background_thread(evt): _thread.start_new_thread(background_thread, (evt,)) evt.wait() assert threading.active_count() == 2, threading.active_count() - if os.fork() == 0: - assert threading.active_count() == 1, threading.active_count() - os._exit(0) - else: - os.wait() + with warnings.catch_warnings(record=True) as ws: + warnings.filterwarnings( + "always", category=DeprecationWarning) + if os.fork() == 0: + assert threading.active_count() == 1, threading.active_count() + os._exit(0) + else: + assert ws[0].category == DeprecationWarning, ws[0] + assert 'fork' in str(ws[0].message), ws[0] + os.wait() """ _, out, err = assert_python_ok("-c", code) self.assertEqual(out, b'') @@ -598,13 +604,15 @@ def test_is_alive_after_fork(self): for i in range(20): t = threading.Thread(target=lambda: None) t.start() - pid = os.fork() - if pid == 0: - os._exit(11 if t.is_alive() else 10) - else: - t.join() + # Ignore the warning about fork with threads. + with warnings.catch_warnings(category=DeprecationWarning, + action="ignore"): + if (pid := os.fork()) == 0: + os._exit(11 if t.is_alive() else 10) + else: + t.join() - support.wait_process(pid, exitcode=10) + support.wait_process(pid, exitcode=10) def test_main_thread(self): main = threading.main_thread() @@ -645,21 +653,26 @@ def test_main_thread_after_fork(self): @unittest.skipUnless(hasattr(os, 'waitpid'), "test needs os.waitpid()") def test_main_thread_after_fork_from_nonmain_thread(self): code = """if 1: - import os, threading, sys + import os, threading, sys, warnings from test import support def func(): - pid = os.fork() - if pid == 0: - main = threading.main_thread() - print(main.name) - print(main.ident == threading.current_thread().ident) - print(main.ident == threading.get_ident()) - # stdout is fully buffered because not a tty, - # we have to flush before exit. - sys.stdout.flush() - else: - support.wait_process(pid, exitcode=0) + with warnings.catch_warnings(record=True) as ws: + warnings.filterwarnings( + "always", category=DeprecationWarning) + pid = os.fork() + if pid == 0: + main = threading.main_thread() + print(main.name) + print(main.ident == threading.current_thread().ident) + print(main.ident == threading.get_ident()) + # stdout is fully buffered because not a tty, + # we have to flush before exit. + sys.stdout.flush() + else: + assert ws[0].category == DeprecationWarning, ws[0] + assert 'fork' in str(ws[0].message), ws[0] + support.wait_process(pid, exitcode=0) th = threading.Thread(target=func) th.start() @@ -667,7 +680,7 @@ def func(): """ _, out, err = assert_python_ok("-c", code) data = out.decode().replace('\r', '') - self.assertEqual(err, b"") + self.assertEqual(err.decode('utf-8'), "") self.assertEqual(data, "Thread-1 (func)\nTrue\nTrue\n") def test_main_thread_during_shutdown(self): @@ -1173,15 +1186,18 @@ def do_fork_and_wait(): else: os._exit(50) - # start a bunch of threads that will fork() child processes - threads = [] - for i in range(16): - t = threading.Thread(target=do_fork_and_wait) - threads.append(t) - t.start() + # Ignore the warning about fork with threads. + with warnings.catch_warnings(category=DeprecationWarning, + action="ignore"): + # start a bunch of threads that will fork() child processes + threads = [] + for i in range(16): + t = threading.Thread(target=do_fork_and_wait) + threads.append(t) + t.start() - for t in threads: - t.join() + for t in threads: + t.join() @support.requires_fork() def test_clear_threads_states_after_fork(self): @@ -1194,18 +1210,22 @@ def test_clear_threads_states_after_fork(self): threads.append(t) t.start() - pid = os.fork() - if pid == 0: - # check that threads states have been cleared - if len(sys._current_frames()) == 1: - os._exit(51) - else: - os._exit(52) - else: - support.wait_process(pid, exitcode=51) - - for t in threads: - t.join() + try: + # Ignore the warning about fork with threads. + with warnings.catch_warnings(category=DeprecationWarning, + action="ignore"): + pid = os.fork() + if pid == 0: + # check that threads states have been cleared + if len(sys._current_frames()) == 1: + os._exit(51) + else: + os._exit(52) + else: + support.wait_process(pid, exitcode=51) + finally: + for t in threads: + t.join() class SubinterpThreadingTests(BaseTestCase): diff --git a/Lib/threading.py b/Lib/threading.py index 723bd58bf5a6..df273870fa42 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -1490,6 +1490,8 @@ def active_count(): enumerate(). """ + # NOTE: if the logic in here ever changes, update Modules/posixmodule.c + # warn_about_fork_with_threads() to match. with _active_limbo_lock: return len(_active) + len(_limbo) diff --git a/Misc/NEWS.d/next/Library/2022-12-13-17-29-09.gh-issue-100228.bgtzMV.rst b/Misc/NEWS.d/next/Library/2022-12-13-17-29-09.gh-issue-100228.bgtzMV.rst new file mode 100644 index 000000000000..ca6e4a2a1f0d --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-12-13-17-29-09.gh-issue-100228.bgtzMV.rst @@ -0,0 +1,5 @@ +A :exc:`DeprecationWarning` may be raised when :func:`os.fork()` or +:func:`os.forkpty()` is called from multi-threaded processes. Forking +with threads is unsafe and can cause deadlocks, crashes and subtle +problems. Lack of a warning does not indicate that the fork call was +actually safe, as Python may not be aware of all threads. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index c32fdb5f5fbe..c777c3efc492 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -25,6 +25,9 @@ #include "structmember.h" // for offsetof(), T_OBJECT #include <float.h> // FLT_MAX #include <signal.h> +#ifndef MS_WINDOWS +#include <unistd.h> +#endif #ifdef HAVE_SYS_WAIT_H #include <sys/wait.h> // W_STOPCODE @@ -871,6 +874,46 @@ test_thread_state(PyObject *self, PyObject *args) Py_RETURN_NONE; } +#ifndef MS_WINDOWS +static PyThread_type_lock wait_done = NULL; + +static void wait_for_lock(void *unused) { + PyThread_acquire_lock(wait_done, 1); + PyThread_release_lock(wait_done); + PyThread_free_lock(wait_done); + wait_done = NULL; +} + +// These can be used to test things that care about the existence of another +// thread that the threading module doesn't know about. + +static PyObject * +spawn_pthread_waiter(PyObject *self, PyObject *Py_UNUSED(ignored)) +{ + if (wait_done) { + PyErr_SetString(PyExc_RuntimeError, "thread already running"); + return NULL; + } + wait_done = PyThread_allocate_lock(); + if (wait_done == NULL) + return PyErr_NoMemory(); + PyThread_acquire_lock(wait_done, 1); + PyThread_start_new_thread(wait_for_lock, NULL); + Py_RETURN_NONE; +} + +static PyObject * +end_spawned_pthread(PyObject *self, PyObject *Py_UNUSED(ignored)) +{ + if (!wait_done) { + PyErr_SetString(PyExc_RuntimeError, "call _spawn_pthread_waiter 1st"); + return NULL; + } + PyThread_release_lock(wait_done); + Py_RETURN_NONE; +} +#endif // not MS_WINDOWS + /* test Py_AddPendingCalls using threads */ static int _pending_callback(void *arg) { @@ -3207,6 +3250,10 @@ static PyMethodDef TestMethods[] = { {"test_get_type_name", test_get_type_name, METH_NOARGS}, {"test_get_type_qualname", test_get_type_qualname, METH_NOARGS}, {"_test_thread_state", test_thread_state, METH_VARARGS}, +#ifndef MS_WINDOWS + {"_spawn_pthread_waiter", spawn_pthread_waiter, METH_NOARGS}, + {"_end_spawned_pthread", end_spawned_pthread, METH_NOARGS}, +#endif {"_pending_threadfunc", pending_threadfunc, METH_VARARGS}, #ifdef HAVE_GETTIMEOFDAY {"profile_int", profile_int, METH_NOARGS}, diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 607d40b59d96..1d9a33ab7bc9 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -72,6 +72,8 @@ */ #if defined(__APPLE__) +#include <mach/mach.h> + #if defined(__has_builtin) #if __has_builtin(__builtin_available) #define HAVE_BUILTIN_AVAILABLE 1 @@ -6745,6 +6747,104 @@ os_register_at_fork_impl(PyObject *module, PyObject *before, } #endif /* HAVE_FORK */ +// Common code to raise a warning if we detect there is more than one thread +// 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) { + // 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. + Py_ssize_t num_python_threads = 0; +#if defined(__APPLE__) && defined(HAVE_GETPID) + mach_port_t macos_self = mach_task_self(); + mach_port_t macos_task; + if (task_for_pid(macos_self, getpid(), &macos_task) == KERN_SUCCESS) { + thread_array_t macos_threads; + mach_msg_type_number_t macos_n_threads; + if (task_threads(macos_task, &macos_threads, + &macos_n_threads) == KERN_SUCCESS) { + num_python_threads = macos_n_threads; + } + } +#elif defined(__linux__) + // Linux /proc/self/stat 20th field is the number of threads. + FILE* proc_stat = fopen("/proc/self/stat", "r"); + if (proc_stat) { + size_t n; + // Size chosen arbitrarily. ~60% more bytes than a 20th column index + // observed on the author's workstation. + char stat_line[160]; + n = fread(&stat_line, 1, 159, proc_stat); + stat_line[n] = '\0'; + fclose(proc_stat); + + char *saveptr = NULL; + char *field = strtok_r(stat_line, " ", &saveptr); + unsigned int idx; + for (idx = 19; idx && field; --idx) { + field = strtok_r(NULL, " ", &saveptr); + } + if (idx == 0 && field) { // found the 20th field + num_python_threads = atoi(field); // 0 on error + } + } +#endif + if (num_python_threads <= 0) { + // Fall back to just the number our threading module knows about. + // An incomplete view of the world, but better than nothing. + PyObject *threading = PyImport_GetModule(&_Py_ID(threading)); + if (!threading) { + PyErr_Clear(); + return; + } + PyObject *threading_active = + PyObject_GetAttr(threading, &_Py_ID(_active)); + if (!threading_active) { + PyErr_Clear(); + Py_DECREF(threading); + return; + } + PyObject *threading_limbo = + PyObject_GetAttr(threading, &_Py_ID(_limbo)); + if (!threading_limbo) { + PyErr_Clear(); + Py_DECREF(threading); + Py_DECREF(threading_active); + return; + } + Py_DECREF(threading); + // Duplicating what threading.active_count() does but without holding + // threading._active_limbo_lock so our count could be inaccurate if + // these dicts are mid-update from another thread. Not a big deal. + // Worst case if someone replaced threading._active or threading._limbo + // with non-dicts, we get -1 from *Length() below and undercount. + // Nobody should, but we're best effort so we clear errors and move on. + num_python_threads = (PyMapping_Length(threading_active) + + PyMapping_Length(threading_limbo)); + PyErr_Clear(); + Py_DECREF(threading_active); + Py_DECREF(threading_limbo); + } + if (num_python_threads > 1) { + PyErr_WarnFormat( + PyExc_DeprecationWarning, 1, +#ifdef HAVE_GETPID + "This process (pid=%d) is multi-threaded, " +#else + "This process is multi-threaded, " +#endif + "use of %s() may lead to deadlocks in the child.", +#ifdef HAVE_GETPID + getpid(), +#endif + name); + PyErr_Clear(); + } +} #ifdef HAVE_FORK1 /*[clinic input] @@ -6771,6 +6871,7 @@ 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(); } @@ -6810,6 +6911,7 @@ 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(); } @@ -7479,6 +7581,7 @@ 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(); }