gh-126688: Support Fork Parent and Child With Different Thread ID by ericsnowcurrently · Pull Request #127121 · python/cpython · GitHub
Skip to content

gh-126688: Support Fork Parent and Child With Different Thread ID #127121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ericsnowcurrently wants to merge 6 commits into
base: main
Choose a base branch
from
+69 −10
Draft

gh-126688: Support Fork Parent and Child With Different Thread ID #127121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
20a9bd6
Hold a global lock while forking.
ericsnowcurrently Nov 21, 2024
259e3c6
Keep track of the parent thread ID.
ericsnowcurrently Nov 21, 2024
8fbd4fc
Update the tstate thread ID if necessary.
ericsnowcurrently Nov 21, 2024
db5271a
Add _PyRecursiveMutex_at_fork_reinit().
ericsnowcurrently Nov 21, 2024
9e015f8
Use _PyRecursiveMutex_at_fork_reinit() in _PyImport_ReInitLock().
ericsnowcurrently Nov 21, 2024
2aad2d5
Call _PyImport_ReInitLock() in _PyRuntimeState_ReInitThreads().
ericsnowcurrently Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Include/internal/pycore_import.h
View file
Open in desktop
18 changes: 17 additions & 1 deletion Include/internal/pycore_lock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,12 @@ _PyOnceFlag_CallOnce(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg)
return _PyOnceFlag_CallOnceSlow(flag, fn, arg);
}

#define _PyThread_ident_t unsigned long long

// A recursive mutex. The mutex should zero-initialized.
typedef struct {
PyMutex mutex;
unsigned long long thread; // i.e., PyThread_get_thread_ident_ex()
_PyThread_ident_t thread; // i.e., PyThread_get_thread_ident_ex()
size_t level;
} _PyRecursiveMutex;

Expand All @@ -164,6 +166,20 @@ extern PyLockStatus _PyRecursiveMutex_LockTimed(_PyRecursiveMutex *m, PyTime_t t
PyAPI_FUNC(void) _PyRecursiveMutex_Unlock(_PyRecursiveMutex *m);
extern int _PyRecursiveMutex_TryUnlock(_PyRecursiveMutex *m);

PyAPI_FUNC(_PyThread_ident_t) PyThread_get_thread_ident_ex(void);

static inline void
_PyRecursiveMutex_at_fork_reinit(_PyRecursiveMutex *m,
_PyThread_ident_t parent)
{
if (m->thread == parent) {
m->thread = PyThread_get_thread_ident_ex();
}
else {
memset(m, 0, sizeof(*m));
}
}

// A readers-writer (RW) lock. The lock supports multiple concurrent readers or
// a single writer. The lock is write-preferring: if a writer is waiting while
// the lock is read-locked then, new readers will be blocked. This avoids
Expand Down
13 changes: 12 additions & 1 deletion Include/internal/pycore_runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,15 @@ typedef struct pyruntimestate {
_Py_AuditHookEntry *head;
} audit_hooks;

#ifdef HAVE_FORK
struct {
PyMutex mutex;
struct _os_fork_parent {
PyThread_ident_t tid;
} parent;
} os_fork;
#endif

struct _py_object_runtime_state object_state;
struct _Py_float_runtime_state float_state;
struct _Py_unicode_runtime_state unicode_state;
Expand Down Expand Up @@ -210,7 +219,9 @@ extern PyStatus _PyRuntimeState_Init(_PyRuntimeState *runtime);
extern void _PyRuntimeState_Fini(_PyRuntimeState *runtime);

#ifdef HAVE_FORK
extern PyStatus _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime);
extern PyStatus _PyRuntimeState_ReInitThreads(
_PyRuntimeState *runtime,
PyThread_ident_t parent);
#endif

/* Initialize _PyRuntimeState.
Expand Down
28 changes: 25 additions & 3 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,12 @@ void
PyOS_BeforeFork(void)
{
PyInterpreterState *interp = _PyInterpreterState_GET();
assert(interp->runtime == &_PyRuntime);
run_at_forkers(interp->before_forkers, 1);

PyMutex_Lock(&_PyRuntime.os_fork.mutex);
_PyRuntime.os_fork.parent.tid = PyThread_get_thread_ident_ex();

_PyImport_AcquireLock(interp);
_PyEval_StopTheWorldAll(&_PyRuntime);
HEAD_LOCK(&_PyRuntime);
Expand All @@ -629,11 +633,17 @@ PyOS_BeforeFork(void)
void
PyOS_AfterFork_Parent(void)
{
_PyRuntime.os_fork.parent = (struct _os_fork_parent){0};

HEAD_UNLOCK(&_PyRuntime);
_PyEval_StartTheWorldAll(&_PyRuntime);

PyInterpreterState *interp = _PyInterpreterState_GET();
assert(interp->runtime == &_PyRuntime);
_PyImport_ReleaseLock(interp);

PyMutex_Unlock(&_PyRuntime.os_fork.mutex);

run_at_forkers(interp->after_forkers_parent, 0);
}

Expand All @@ -644,15 +654,25 @@ PyOS_AfterFork_Child(void)
_PyRuntimeState *runtime = &_PyRuntime;

// re-creates runtime->interpreters.mutex (HEAD_UNLOCK)
status = _PyRuntimeState_ReInitThreads(runtime);
status = _PyRuntimeState_ReInitThreads(
runtime, runtime->os_fork.parent.tid);
if (_PyStatus_EXCEPTION(status)) {
goto fatal_error;
}

PyThreadState *tstate = _PyThreadState_GET();
_Py_EnsureTstateNotNULL(tstate);
assert(tstate->interp->runtime == &_PyRuntime);

assert(tstate->thread_id == PyThread_get_thread_ident());
// We cannot assume that the parent and child always have
// the same thread ID.
// See https://github.com/python/cpython/issues/126688.
if (_PyRuntime.os_fork.parent.tid != PyThread_get_thread_ident_ex()) {
tstate->thread_id = PyThread_get_thread_ident();
}
else {
assert(tstate->thread_id == PyThread_get_thread_ident());
}
#ifdef PY_HAVE_THREAD_NATIVE_ID
tstate->native_thread_id = PyThread_get_thread_native_id();
#endif
Expand All @@ -678,7 +698,6 @@ PyOS_AfterFork_Child(void)
_PyEval_StartTheWorldAll(&_PyRuntime);
_PyThreadState_DeleteList(list);

_PyImport_ReInitLock(tstate->interp);

@gpshead gpshead Nov 23, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment mentioning which call above is now responsible for doing this.

_PyImport_ReleaseLock(tstate->interp);

_PySignal_AfterFork();
Expand All @@ -694,6 +713,9 @@ PyOS_AfterFork_Child(void)
goto fatal_error;
}

_PyRuntime.os_fork.parent = (struct _os_fork_parent){0};
PyMutex_Unlock(&_PyRuntime.os_fork.mutex);

@gpshead gpshead Nov 23, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might need to be done before _PyThreadState_DeleteList above to handle the very silly case of someone forking from a destructor and deadlocking? ("why is that even reasonable to support?" is valid...)


run_at_forkers(tstate->interp->after_forkers_child, 0);
return;

Expand Down
9 changes: 6 additions & 3 deletions Python/import.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,15 @@ _PyImport_ReleaseLock(PyInterpreterState *interp)
_PyRecursiveMutex_Unlock(&IMPORT_LOCK(interp));
}

#ifdef HAVE_FORK
void
_PyImport_ReInitLock(PyInterpreterState *interp)
_PyImport_ReInitLock(PyInterpreterState *interp, PyThread_ident_t parent)
{
// gh-126688: Thread id may change after fork() on some operating systems.
IMPORT_LOCK(interp).thread = PyThread_get_thread_ident_ex();
/* The forking thread always holds the import lock. */
assert(IMPORT_LOCK(interp).thread == parent);
_PyRecursiveMutex_at_fork_reinit(&IMPORT_LOCK(interp), parent);
}
#endif


/***************/
Expand Down
4 changes: 3 additions & 1 deletion Python/pystate.c
View file
Open in desktop