From b5a53af9bd7529566ec2c5715862db2863ce1b76 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Tue, 13 Dec 2022 17:41:39 -0800 Subject: [PATCH 01/18] Warn from os.fork() if other threads exist. Not comprehensive, best effort. --- Lib/threading.py | 2 + ...-12-13-17-29-09.gh-issue-100228.bgtzMV.rst | 4 ++ Modules/posixmodule.c | 49 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-12-13-17-29-09.gh-issue-100228.bgtzMV.rst diff --git a/Lib/threading.py b/Lib/threading.py index 723bd58bf5a68b..df273870fa4273 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 00000000000000..37182d962ced86 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-12-13-17-29-09.gh-issue-100228.bgtzMV.rst @@ -0,0 +1,4 @@ +A :exc:`DeprecationWarning` may be raised when :func:`os.fork()` or +:func:`os.forkpty()` is called from multi-threaded processes. This is to +help people realize they are doing something unsafe. Lack of this seeing a +warning does not imply safety. diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 4817973262f484..46c455c35b72b0 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6737,6 +6737,52 @@ os_register_at_fork_impl(PyObject *module, PyObject *before, } #endif /* HAVE_FORK */ +// Common code to raise a warning if we know there is more than one thread +// running in the process. Best effort, silent if unable to count threads. +// Constraint: Acquires no locks. Quick. Never leaves an error set. +void warn_about_fork_with_threads(const char* name) { + // TODO: Call native OS platform APIs to determine the number of threads in + // the process. Threads launched by Python itself are only one part of the + // story. System libraries and extension modules or embedding code are other + // common sources of threads. + PyObject *threading = PyImport_GetModule(&_Py_ID(threading)); + if (!threading) { + PyErr_Clear(); + return; + } + PyObject *threading_active = PyObject_GetAttrString(threading, "_active"); + if (!threading_active) { + PyErr_Clear(); + Py_DECREF(threading); + return; + } + PyObject *threading_limbo = PyObject_GetAttrString(threading, "_limbo"); + if (!threading_limbo) { + goto end; + } + // Worst case if someone replaced threading._active or threading._limbo + // with non-dicts, we get -1 from *Length() below and undercount. + // Whatever. That shouldn't happen, we're best effort so we clear errors + // and move on. + assert(PyMapping_Check(threading_active)); + assert(PyMapping_Check(threading_limbo)); + // 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. + Py_ssize_t num_python_threads = (PyMapping_Length(threading_active) + + PyMapping_Length(threading_limbo)); + PyErr_Clear(); + if (num_python_threads > 1) { + PyErr_WarnFormat( + PyExc_DeprecationWarning, 1, + "multi-threaded process, %s() may cause deadlocks.", name); + } +end: + PyErr_Clear(); + Py_XDECREF(threading); + Py_XDECREF(threading_active); + Py_XDECREF(threading_limbo); +} #ifdef HAVE_FORK1 /*[clinic input] @@ -6758,6 +6804,7 @@ os_fork1_impl(PyObject *module) return NULL; } PyOS_BeforeFork(); + warn_about_fork_with_threads("fork1"); pid = fork1(); if (pid == 0) { /* child: this clobbers and resets the import lock. */ @@ -6797,6 +6844,7 @@ os_fork_impl(PyObject *module) return NULL; } PyOS_BeforeFork(); + warn_about_fork_with_threads("fork"); pid = fork(); if (pid == 0) { /* child: this clobbers and resets the import lock. */ @@ -7466,6 +7514,7 @@ os_forkpty_impl(PyObject *module) return NULL; } PyOS_BeforeFork(); + warn_about_fork_with_threads("forkpty"); pid = forkpty(&master_fd, NULL, NULL, NULL); if (pid == 0) { /* child: this clobbers and resets the import lock. */ From f4a1dda246a9fb0fe5365b8c491112b442f370db Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 14 Dec 2022 06:14:32 +0000 Subject: [PATCH 02/18] fix existing tests for the new warning. --- Lib/test/test_threading.py | 48 +++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 13ba5068ae20d7..f60ed9b86ca1f8 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -563,7 +563,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 +575,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'') @@ -645,21 +650,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 +677,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): From bca33f937890f65f728c94c4e73992552409820a Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 14 Dec 2022 06:30:10 +0000 Subject: [PATCH 03/18] static --- Modules/posixmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 46c455c35b72b0..e15459233ca2eb 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6740,7 +6740,7 @@ os_register_at_fork_impl(PyObject *module, PyObject *before, // Common code to raise a warning if we know there is more than one thread // running in the process. Best effort, silent if unable to count threads. // Constraint: Acquires no locks. Quick. Never leaves an error set. -void warn_about_fork_with_threads(const char* name) { +static void warn_about_fork_with_threads(const char* name) { // TODO: Call native OS platform APIs to determine the number of threads in // the process. Threads launched by Python itself are only one part of the // story. System libraries and extension modules or embedding code are other From d0eb2f669fe02356148df6eda290ebf2f38c9ce0 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 14 Dec 2022 06:50:14 +0000 Subject: [PATCH 04/18] &_Py_ID() for _active and _limbo. --- Include/internal/pycore_global_objects_fini_generated.h | 2 ++ Include/internal/pycore_global_strings.h | 2 ++ Include/internal/pycore_runtime_init_generated.h | 2 ++ Include/internal/pycore_unicodeobject_generated.h | 4 ++++ Modules/posixmodule.c | 4 ++-- 5 files changed, 12 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.h index 6aba2f19ebde4a..a5365d53150b4f 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 acb9a4fbb92dce..3d9e61e50eccaa 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 6d1b8702c77698..3534b94753e0d3 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 7f407c0141b8a5..02435071bb2cff 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/Modules/posixmodule.c b/Modules/posixmodule.c index e15459233ca2eb..80a7eba80b04a8 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6750,13 +6750,13 @@ static void warn_about_fork_with_threads(const char* name) { PyErr_Clear(); return; } - PyObject *threading_active = PyObject_GetAttrString(threading, "_active"); + PyObject *threading_active = PyObject_GetAttr(threading, &_Py_ID(_active)); if (!threading_active) { PyErr_Clear(); Py_DECREF(threading); return; } - PyObject *threading_limbo = PyObject_GetAttrString(threading, "_limbo"); + PyObject *threading_limbo = PyObject_GetAttr(threading, &_Py_ID(_limbo)); if (!threading_limbo) { goto end; } From fb098f16aaacfd08941f07f1862cb6dcd1f038bc Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 13 Dec 2022 22:55:53 -0800 Subject: [PATCH 05/18] news wording tweak --- .../Library/2022-12-13-17-29-09.gh-issue-100228.bgtzMV.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index 37182d962ced86..4a71f05bf5aa53 100644 --- 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 @@ -1,4 +1,4 @@ A :exc:`DeprecationWarning` may be raised when :func:`os.fork()` or :func:`os.forkpty()` is called from multi-threaded processes. This is to -help people realize they are doing something unsafe. Lack of this seeing a -warning does not imply safety. +help people realize they are doing something unsafe. Lack of a warning +does not indicate that the fork call was actually safe. From 67e2b756423b6d62e8debda920b5f7dc393101a8 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 14 Dec 2022 09:47:38 +0000 Subject: [PATCH 06/18] Add platform API calls on Linux & Mac, polish up. * Only warn in the parent process. * Add a test to test_os that a non-Python thread triggers the warning. * Avoid the system calls to count threads if we've already warned once. * The macOS API calls were written blind based on docs, will fix later if CI fails. --- Lib/test/test_os.py | 25 ++++++++ Modules/_testcapimodule.c | 46 +++++++++++++++ Modules/posixmodule.c | 116 ++++++++++++++++++++++++++------------ 3 files changed, 152 insertions(+), 35 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index e0577916428a08..50bf273b92dd06 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -4534,6 +4534,31 @@ 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 len(ws) == 0 # Warn only the parent + os._exit(0) # child + else: + assert ws[0].category == DeprecationWarning, ws[0] + assert 'fork' in str(ws[0].message), ws[0] + assert threading.active_count() == 1, threading.enumerate() + finally: + _end_spawned_pthread() + """ + _, out, err = assert_python_ok("-c", code) + 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/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 83eef73a875d9d..68d5b255d8836f 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -24,6 +24,9 @@ #include "structmember.h" // for offsetof(), T_OBJECT #include // FLT_MAX #include +#ifndef MS_WINDOWS +#include +#endif #ifdef HAVE_SYS_WAIT_H #include // W_STOPCODE @@ -870,6 +873,45 @@ 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); +} + +// These can be used to test things that care about the existence of another +// thread that the Python runtime 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); + PyThread_free_lock(wait_done); + wait_done = NULL; + Py_RETURN_NONE; +} +#endif // not MS_WINDOWS + /* test Py_AddPendingCalls using threads */ static int _pending_callback(void *arg) { @@ -3262,6 +3304,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 80a7eba80b04a8..3e1a3bfe6efc47 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -72,6 +72,8 @@ */ #if defined(__APPLE__) +#include + #if defined(__has_builtin) #if __has_builtin(__builtin_available) #define HAVE_BUILTIN_AVAILABLE 1 @@ -6739,49 +6741,93 @@ os_register_at_fork_impl(PyObject *module, PyObject *before, // Common code to raise a warning if we know there is more than one thread // running in the process. Best effort, silent if unable to count threads. -// Constraint: Acquires no locks. Quick. Never leaves an error set. +// Constraint: Avoids locks. Quick. Never leaves an error set. static void warn_about_fork_with_threads(const char* name) { - // TODO: Call native OS platform APIs to determine the number of threads in - // the process. Threads launched by Python itself are only one part of the - // story. System libraries and extension modules or embedding code are other - // common sources of threads. - PyObject *threading = PyImport_GetModule(&_Py_ID(threading)); - if (!threading) { - PyErr_Clear(); + static pid_t already_warned = 0; +#if defined(HAVE_GETPID) + if (getpid() == already_warned) { + // Avoid any system calls to reconfirm in this process if the warning might + // be silenced. return; } - PyObject *threading_active = PyObject_GetAttr(threading, &_Py_ID(_active)); - if (!threading_active) { +#endif + 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, NULL /* &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; + 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 of threads this CPython runtime knows about. + 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_XDECREF(threading); + Py_XDECREF(threading_active); + return; + } + // Worst case if someone replaced threading._active or threading._limbo + // with non-dicts, we get -1 from *Length() below and undercount. + // Whatever. That shouldn't happen, we're best effort so we clear errors + // and move on. + assert(PyMapping_Check(threading_active)); + assert(PyMapping_Check(threading_limbo)); + // 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. + num_python_threads = (PyMapping_Length(threading_active) + + PyMapping_Length(threading_limbo)); PyErr_Clear(); Py_DECREF(threading); - return; + Py_DECREF(threading_active); + Py_DECREF(threading_limbo); } - PyObject *threading_limbo = PyObject_GetAttr(threading, &_Py_ID(_limbo)); - if (!threading_limbo) { - goto end; - } - // Worst case if someone replaced threading._active or threading._limbo - // with non-dicts, we get -1 from *Length() below and undercount. - // Whatever. That shouldn't happen, we're best effort so we clear errors - // and move on. - assert(PyMapping_Check(threading_active)); - assert(PyMapping_Check(threading_limbo)); - // 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. - Py_ssize_t num_python_threads = (PyMapping_Length(threading_active) + - PyMapping_Length(threading_limbo)); - PyErr_Clear(); if (num_python_threads > 1) { PyErr_WarnFormat( PyExc_DeprecationWarning, 1, "multi-threaded process, %s() may cause deadlocks.", name); +#ifdef HAVE_GETPID + already_warned = getpid(); +#endif } -end: - PyErr_Clear(); - Py_XDECREF(threading); - Py_XDECREF(threading_active); - Py_XDECREF(threading_limbo); } #ifdef HAVE_FORK1 @@ -6804,12 +6850,12 @@ os_fork1_impl(PyObject *module) return NULL; } PyOS_BeforeFork(); - warn_about_fork_with_threads("fork1"); pid = fork1(); if (pid == 0) { /* 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(); } @@ -6844,12 +6890,12 @@ os_fork_impl(PyObject *module) return NULL; } PyOS_BeforeFork(); - warn_about_fork_with_threads("fork"); pid = fork(); if (pid == 0) { /* 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(); } @@ -7514,12 +7560,12 @@ os_forkpty_impl(PyObject *module) return NULL; } PyOS_BeforeFork(); - warn_about_fork_with_threads("forkpty"); pid = forkpty(&master_fd, NULL, NULL, NULL); if (pid == 0) { /* 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(); } From 203188803cd4ad72ceb64c5a4d9bad0aca7e17f4 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 14 Dec 2022 10:10:42 +0000 Subject: [PATCH 07/18] Probably fix the macOS crashes. I don't want the array of thread info but it may insist? --- Modules/posixmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 3e1a3bfe6efc47..e25fc86061790e 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6756,9 +6756,9 @@ static void warn_about_fork_with_threads(const char* name) { 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; + thread_array_t macos_threads; mach_msg_type_number_t macos_n_threads; - if (task_threads(macos_task, NULL /* &macos_threads */, + if (task_threads(macos_task, &macos_threads, &macos_n_threads) == KERN_SUCCESS) { num_python_threads = macos_n_threads; } From 546fac1704489a7063a0a412adeacf3501437c1c Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 14 Dec 2022 19:16:41 -0800 Subject: [PATCH 08/18] Fix test_os race condition. Let the thread free and clear things as it ends after we unblock it. --- Modules/_testcapimodule.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 68d5b255d8836f..a5746784713334 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -878,6 +878,9 @@ 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 @@ -906,8 +909,6 @@ end_spawned_pthread(PyObject *self, PyObject *Py_UNUSED(ignored)) return NULL; } PyThread_release_lock(wait_done); - PyThread_free_lock(wait_done); - wait_done = NULL; Py_RETURN_NONE; } #endif // not MS_WINDOWS From a2e753e23aa33660dcb428f30d4d09ac28b752cf Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 14 Dec 2022 19:18:38 -0800 Subject: [PATCH 09/18] remove the already_warned logic. It could hide more than we intend and wouldn't do what I wanted in child processes that inherit the parents warnings state anyways. --- Modules/posixmodule.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index e25fc86061790e..b4954ecf483352 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6743,14 +6743,6 @@ os_register_at_fork_impl(PyObject *module, PyObject *before, // running in the process. Best effort, silent if unable to count threads. // Constraint: Avoids locks. Quick. Never leaves an error set. static void warn_about_fork_with_threads(const char* name) { - static pid_t already_warned = 0; -#if defined(HAVE_GETPID) - if (getpid() == already_warned) { - // Avoid any system calls to reconfirm in this process if the warning might - // be silenced. - return; - } -#endif Py_ssize_t num_python_threads = 0; #if defined(__APPLE__) && defined(HAVE_GETPID) mach_port_t macos_self = mach_task_self(); @@ -6824,9 +6816,6 @@ static void warn_about_fork_with_threads(const char* name) { PyErr_WarnFormat( PyExc_DeprecationWarning, 1, "multi-threaded process, %s() may cause deadlocks.", name); -#ifdef HAVE_GETPID - already_warned = getpid(); -#endif } } From 5655d59c3c3df6192414db6436ff79012cdaae97 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 14 Dec 2022 19:25:57 -0800 Subject: [PATCH 10/18] Improve the warning message. --- Modules/posixmodule.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index b4954ecf483352..e0f1cb689fdd83 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6815,7 +6815,8 @@ static void warn_about_fork_with_threads(const char* name) { if (num_python_threads > 1) { PyErr_WarnFormat( PyExc_DeprecationWarning, 1, - "multi-threaded process, %s() may cause deadlocks.", name); + "multi-threaded process detected, " + "use of %s() may lead to deadlocks in the child.", name); } } From 7c9205e7f063cf9787cae90c9a4b36dacc51b9ba Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Wed, 14 Dec 2022 19:41:41 -0800 Subject: [PATCH 11/18] Update Misc/NEWS.d/next/Library/2022-12-13-17-29-09.gh-issue-100228.bgtzMV.rst Co-authored-by: T. Wouters --- .../Library/2022-12-13-17-29-09.gh-issue-100228.bgtzMV.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 index 4a71f05bf5aa53..ca6e4a2a1f0d2e 100644 --- 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 @@ -1,4 +1,5 @@ A :exc:`DeprecationWarning` may be raised when :func:`os.fork()` or -:func:`os.forkpty()` is called from multi-threaded processes. This is to -help people realize they are doing something unsafe. Lack of a warning -does not indicate that the fork call was actually safe. +: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. From 28544731190331bebae1b135b3fd1abb4b835a37 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Wed, 14 Dec 2022 19:47:54 -0800 Subject: [PATCH 12/18] Update a comment Co-authored-by: T. Wouters --- Modules/_testcapimodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index a5746784713334..52272ac3ffe285 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -884,7 +884,7 @@ static void wait_for_lock(void *unused) { } // These can be used to test things that care about the existence of another -// thread that the Python runtime doesn't know about. +// thread that the threading module doesn't know about. static PyObject * spawn_pthread_waiter(PyObject *self, PyObject *Py_UNUSED(ignored)) From 1d3ec86a3382fc656cc04b8587056d42a03fdb22 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 14 Dec 2022 20:12:32 -0800 Subject: [PATCH 13/18] Clean up comments; add a final PyErr_Clear warning --- Modules/posixmodule.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index e0f1cb689fdd83..01e9d2ef4a132d 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6739,10 +6739,13 @@ os_register_at_fork_impl(PyObject *module, PyObject *before, } #endif /* HAVE_FORK */ -// Common code to raise a warning if we know there is more than one thread -// running in the process. Best effort, silent if unable to count threads. -// Constraint: Avoids locks. Quick. Never leaves an error set. +// 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. 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(); @@ -6760,6 +6763,8 @@ static void warn_about_fork_with_threads(const char* name) { 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'; @@ -6777,7 +6782,8 @@ static void warn_about_fork_with_threads(const char* name) { } #endif if (num_python_threads <= 0) { - // Fall back to just the number of threads this CPython runtime knows about. + // Fall back to just the number of threads our threading module knows about. + // An incomplete view of the world, but better than nothing for our purpose. PyObject *threading = PyImport_GetModule(&_Py_ID(threading)); if (!threading) { PyErr_Clear(); @@ -6796,19 +6802,16 @@ static void warn_about_fork_with_threads(const char* name) { Py_XDECREF(threading_active); return; } - // Worst case if someone replaced threading._active or threading._limbo - // with non-dicts, we get -1 from *Length() below and undercount. - // Whatever. That shouldn't happen, we're best effort so we clear errors - // and move on. - assert(PyMapping_Check(threading_active)); - assert(PyMapping_Check(threading_limbo)); + 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); Py_DECREF(threading_active); Py_DECREF(threading_limbo); } @@ -6817,6 +6820,7 @@ static void warn_about_fork_with_threads(const char* name) { PyExc_DeprecationWarning, 1, "multi-threaded process detected, " "use of %s() may lead to deadlocks in the child.", name); + PyErr_Clear(); } } From 0d956793aefcf6d3d4720b9eb5dba95d6857ff52 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 14 Dec 2022 20:29:19 -0800 Subject: [PATCH 14/18] Improve test_os robustness. --- Lib/test/test_os.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 50bf273b92dd06..94fc4ccb196465 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -4546,16 +4546,19 @@ def test_fork_warns_when_non_python_thread_exists(self): warnings.filterwarnings( "always", category=DeprecationWarning) if os.fork() == 0: - assert len(ws) == 0 # Warn only the parent + 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) + _, out, err = assert_python_ok("-c", code, PYTHONOPTIMIZE='0') self.assertEqual(err.decode("utf-8"), "") self.assertEqual(out.decode("utf-8"), "") From f6a7b6e5e29c647119b31a3319ba096c224cbb7f Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 14 Dec 2022 20:49:53 -0800 Subject: [PATCH 15/18] ignore expected warnings. --- Lib/test/test_threading.py | 48 ++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index f60ed9b86ca1f8..530b7961ccca45 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 @@ -1183,15 +1184,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): @@ -1204,18 +1208,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): From 0ca9fc7caf870ac80085e29c739e9b00be4ffd2f Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 14 Dec 2022 20:59:17 -0800 Subject: [PATCH 16/18] improve error message. --- Modules/posixmodule.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 01e9d2ef4a132d..0d9c59b999f61b 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6818,8 +6818,16 @@ static void warn_about_fork_with_threads(const char* name) { if (num_python_threads > 1) { PyErr_WarnFormat( PyExc_DeprecationWarning, 1, - "multi-threaded process detected, " - "use of %s() may lead to deadlocks in the child.", name); +#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(); } } From a91a4d10c391004567c7030944a8a7643512a27e Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 14 Dec 2022 21:19:28 -0800 Subject: [PATCH 17/18] silence the warning in more tests. --- Lib/test/fork_wait.py | 31 +++++++++++++++---------------- Lib/test/test_thread.py | 13 ++++++++----- Lib/test/test_threading.py | 14 ++++++++------ 3 files changed, 31 insertions(+), 27 deletions(-) diff --git a/Lib/test/fork_wait.py b/Lib/test/fork_wait.py index ebd07e612e5810..c26c7aaaeb4306 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_thread.py b/Lib/test/test_thread.py index 2ae5e9ccb44088..8656fbdd83e9c7 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 530b7961ccca45..31bf46311a80dc 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -604,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() From dc8fe086e7510fc96cdc1e359b3e07b96e4bca3c Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Wed, 28 Dec 2022 17:38:47 -0800 Subject: [PATCH 18/18] line length fixes, comments++, DECREF vs XDECREF --- Modules/posixmodule.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 0d9c59b999f61b..1f27bdb66bc67e 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6742,6 +6742,10 @@ os_register_at_fork_impl(PyObject *module, PyObject *before, // 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 @@ -6782,30 +6786,32 @@ static void warn_about_fork_with_threads(const char* name) { } #endif if (num_python_threads <= 0) { - // Fall back to just the number of threads our threading module knows about. - // An incomplete view of the world, but better than nothing for our purpose. + // 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)); + 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)); + PyObject *threading_limbo = + PyObject_GetAttr(threading, &_Py_ID(_limbo)); if (!threading_limbo) { PyErr_Clear(); - Py_XDECREF(threading); - Py_XDECREF(threading_active); + 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. + // 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.