Skip to content

Commit 3dd35aa

Browse files
gpsheadclaude
authored andcommitted
pythongh-137109: refactor warning about threads when forking (python#141438)
* pythongh-137109: refactor warning about threads when forking This splits the OS API specific functionality to get the number of threads out from the fallback Python method and warning raising code itself. This way the OS APIs can be queried before we've run `os.register_at_fork(after_in_parent=...)` registered functions which themselves may (re)start threads that would otherwise be detected. This is best effort. If the OS APIs are either unavailable or fail, the warning generating code still falls back to looking at the Python threading state after the CPython interpreter world has been restarted and the after_in_parent calls have been made. The common case for most Linux and macOS environments should work today. This also lines up with the existing TODO refactoring, we may choose to expose this API to get the number of OS threads in the `os` module in the future. * NEWS entry * avoid "function-prototype" compiler warning?
1 parent 79c136f commit 3dd35aa

2 files changed

Lines changed: 75 additions & 49 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
The :mod:`os.fork` and related forking APIs will no longer warn in the
2+
common case where Linux or macOS platform APIs return the number of threads
3+
in a process and find the answer to be 1 even when a
4+
:func:`os.register_at_fork` ``after_in_parent=`` callback (re)starts a
5+
thread.

Modules/posixmodule.c

Lines changed: 70 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -7975,75 +7975,41 @@ os_register_at_fork_impl(PyObject *module, PyObject *before,
79757975
// running in the process. Best effort, silent if unable to count threads.
79767976
// Constraint: Quick. Never overcounts. Never leaves an error set.
79777977
//
7978-
// This should only be called from the parent process after
7978+
// This MUST only be called from the parent process after
79797979
// PyOS_AfterFork_Parent().
7980-
static void
7981-
warn_about_fork_with_threads(const char* name)
7980+
static int
7981+
warn_about_fork_with_threads(
7982+
const char* name, // Name of the API to use in the warning message.
7983+
const Py_ssize_t num_os_threads // Only trusted when >= 1.
7984+
)
79827985
{
79837986
// It's not safe to issue the warning while the world is stopped, because
79847987
// other threads might be holding locks that we need, which would deadlock.
79857988
assert(!_PyRuntime.stoptheworld.world_stopped);
79867989

7987-
// TODO: Consider making an `os` module API to return the current number
7988-
// of threads in the process. That'd presumably use this platform code but
7989-
// raise an error rather than using the inaccurate fallback.
7990-
Py_ssize_t num_python_threads = 0;
7991-
#if defined(__APPLE__) && defined(HAVE_GETPID)
7992-
mach_port_t macos_self = mach_task_self();
7993-
mach_port_t macos_task;
7994-
if (task_for_pid(macos_self, getpid(), &macos_task) == KERN_SUCCESS) {
7995-
thread_array_t macos_threads;
7996-
mach_msg_type_number_t macos_n_threads;
7997-
if (task_threads(macos_task, &macos_threads,
7998-
&macos_n_threads) == KERN_SUCCESS) {
7999-
num_python_threads = macos_n_threads;
8000-
}
8001-
}
8002-
#elif defined(__linux__)
8003-
// Linux /proc/self/stat 20th field is the number of threads.
8004-
FILE* proc_stat = fopen("/proc/self/stat", "r");
8005-
if (proc_stat) {
8006-
size_t n;
8007-
// Size chosen arbitrarily. ~60% more bytes than a 20th column index
8008-
// observed on the author's workstation.
8009-
char stat_line[160];
8010-
n = fread(&stat_line, 1, 159, proc_stat);
8011-
stat_line[n] = '\0';
8012-
fclose(proc_stat);
8013-
8014-
char *saveptr = NULL;
8015-
char *field = strtok_r(stat_line, " ", &saveptr);
8016-
unsigned int idx;
8017-
for (idx = 19; idx && field; --idx) {
8018-
field = strtok_r(NULL, " ", &saveptr);
8019-
}
8020-
if (idx == 0 && field) { // found the 20th field
8021-
num_python_threads = atoi(field); // 0 on error
8022-
}
8023-
}
8024-
#endif
7990+
Py_ssize_t num_python_threads = num_os_threads;
80257991
if (num_python_threads <= 0) {
80267992
// Fall back to just the number our threading module knows about.
80277993
// An incomplete view of the world, but better than nothing.
80287994
PyObject *threading = PyImport_GetModule(&_Py_ID(threading));
80297995
if (!threading) {
80307996
PyErr_Clear();
8031-
return;
7997+
return 0;
80327998
}
80337999
PyObject *threading_active =
80348000
PyObject_GetAttr(threading, &_Py_ID(_active));
80358001
if (!threading_active) {
80368002
PyErr_Clear();
80378003
Py_DECREF(threading);
8038-
return;
8004+
return 0;
80398005
}
80408006
PyObject *threading_limbo =
80418007
PyObject_GetAttr(threading, &_Py_ID(_limbo));
80428008
if (!threading_limbo) {
80438009
PyErr_Clear();
80448010
Py_DECREF(threading);
80458011
Py_DECREF(threading_active);
8046-
return;
8012+
return 0;
80478013
}
80488014
Py_DECREF(threading);
80498015
// Duplicating what threading.active_count() does but without holding
@@ -8059,7 +8025,7 @@ warn_about_fork_with_threads(const char* name)
80598025
Py_DECREF(threading_limbo);
80608026
}
80618027
if (num_python_threads > 1) {
8062-
PyErr_WarnFormat(
8028+
return PyErr_WarnFormat(
80638029
PyExc_DeprecationWarning, 1,
80648030
#ifdef HAVE_GETPID
80658031
"This process (pid=%d) is multi-threaded, "
@@ -8071,8 +8037,53 @@ warn_about_fork_with_threads(const char* name)
80718037
getpid(),
80728038
#endif
80738039
name);
8074-
PyErr_Clear();
80758040
}
8041+
return 0;
8042+
}
8043+
8044+
// If this returns <= 0, we were unable to successfully use any OS APIs.
8045+
// Returns a positive number of threads otherwise.
8046+
static Py_ssize_t get_number_of_os_threads(void)
8047+
{
8048+
// TODO: Consider making an `os` module API to return the current number
8049+
// of threads in the process. That'd presumably use this platform code but
8050+
// raise an error rather than using the inaccurate fallback.
8051+
Py_ssize_t num_python_threads = 0;
8052+
#if defined(__APPLE__) && defined(HAVE_GETPID)
8053+
mach_port_t macos_self = mach_task_self();
8054+
mach_port_t macos_task;
8055+
if (task_for_pid(macos_self, getpid(), &macos_task) == KERN_SUCCESS) {
8056+
thread_array_t macos_threads;
8057+
mach_msg_type_number_t macos_n_threads;
8058+
if (task_threads(macos_task, &macos_threads,
8059+
&macos_n_threads) == KERN_SUCCESS) {
8060+
num_python_threads = macos_n_threads;
8061+
}
8062+
}
8063+
#elif defined(__linux__)
8064+
// Linux /proc/self/stat 20th field is the number of threads.
8065+
FILE* proc_stat = fopen("/proc/self/stat", "r");
8066+
if (proc_stat) {
8067+
size_t n;
8068+
// Size chosen arbitrarily. ~60% more bytes than a 20th column index
8069+
// observed on the author's workstation.
8070+
char stat_line[160];
8071+
n = fread(&stat_line, 1, 159, proc_stat);
8072+
stat_line[n] = '\0';
8073+
fclose(proc_stat);
8074+
8075+
char *saveptr = NULL;
8076+
char *field = strtok_r(stat_line, " ", &saveptr);
8077+
unsigned int idx;
8078+
for (idx = 19; idx && field; --idx) {
8079+
field = strtok_r(NULL, " ", &saveptr);
8080+
}
8081+
if (idx == 0 && field) { // found the 20th field
8082+
num_python_threads = atoi(field); // 0 on error
8083+
}
8084+
}
8085+
#endif
8086+
return num_python_threads;
80768087
}
80778088
#endif // HAVE_FORK1 || HAVE_FORKPTY || HAVE_FORK
80788089

@@ -8108,10 +8119,14 @@ os_fork1_impl(PyObject *module)
81088119
/* child: this clobbers and resets the import lock. */
81098120
PyOS_AfterFork_Child();
81108121
} else {
8122+
// Called before AfterFork_Parent in case those hooks start threads.
8123+
Py_ssize_t num_os_threads = get_number_of_os_threads();
81118124
/* parent: release the import lock. */
81128125
PyOS_AfterFork_Parent();
81138126
// After PyOS_AfterFork_Parent() starts the world to avoid deadlock.
8114-
warn_about_fork_with_threads("fork1");
8127+
if (warn_about_fork_with_threads("fork1", num_os_threads) < 0) {
8128+
return NULL;
8129+
}
81158130
}
81168131
if (pid == -1) {
81178132
errno = saved_errno;
@@ -8157,10 +8172,13 @@ os_fork_impl(PyObject *module)
81578172
/* child: this clobbers and resets the import lock. */
81588173
PyOS_AfterFork_Child();
81598174
} else {
8175+
// Called before AfterFork_Parent in case those hooks start threads.
8176+
Py_ssize_t num_os_threads = get_number_of_os_threads();
81608177
/* parent: release the import lock. */
81618178
PyOS_AfterFork_Parent();
81628179
// After PyOS_AfterFork_Parent() starts the world to avoid deadlock.
8163-
warn_about_fork_with_threads("fork");
8180+
if (warn_about_fork_with_threads("fork", num_os_threads) < 0)
8181+
return NULL;
81648182
}
81658183
if (pid == -1) {
81668184
errno = saved_errno;
@@ -9014,10 +9032,13 @@ os_forkpty_impl(PyObject *module)
90149032
/* child: this clobbers and resets the import lock. */
90159033
PyOS_AfterFork_Child();
90169034
} else {
9035+
// Called before AfterFork_Parent in case those hooks start threads.
9036+
Py_ssize_t num_os_threads = get_number_of_os_threads();
90179037
/* parent: release the import lock. */
90189038
PyOS_AfterFork_Parent();
90199039
// After PyOS_AfterFork_Parent() starts the world to avoid deadlock.
9020-
warn_about_fork_with_threads("forkpty");
9040+
if (warn_about_fork_with_threads("forkpty", num_os_threads) < 0)
9041+
return NULL;
90219042
}
90229043
if (pid == -1) {
90239044
return posix_error();

0 commit comments

Comments
 (0)