Skip to content

Commit e21057b

Browse files
authored
gh-117657: Fix TSAN race involving import lock (#118523)
This adds a `_PyRecursiveMutex` type based on `PyMutex` and uses that for the import lock. This fixes some data races in the free-threaded build and generally simplifies the import lock code.
1 parent 417bec7 commit e21057b

File tree

7 files changed

+90
-105
lines changed

7 files changed

+90
-105
lines changed

Include/internal/pycore_import.h

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ PyAPI_FUNC(int) _PyImport_SetModule(PyObject *name, PyObject *module);
2020
extern int _PyImport_SetModuleString(const char *name, PyObject* module);
2121

2222
extern void _PyImport_AcquireLock(PyInterpreterState *interp);
23-
extern int _PyImport_ReleaseLock(PyInterpreterState *interp);
23+
extern void _PyImport_ReleaseLock(PyInterpreterState *interp);
2424

2525
// This is used exclusively for the sys and builtins modules:
2626
extern int _PyImport_FixupBuiltin(
@@ -94,11 +94,7 @@ struct _import_state {
9494
#endif
9595
PyObject *import_func;
9696
/* The global import lock. */
97-
struct {
98-
PyThread_type_lock mutex;
99-
unsigned long thread;
100-
int level;
101-
} lock;
97+
_PyRecursiveMutex lock;
10298
/* diagnostic info in PyImport_ImportModuleLevelObject() */
10399
struct {
104100
int import_level;
@@ -123,11 +119,6 @@ struct _import_state {
123119
#define IMPORTS_INIT \
124120
{ \
125121
DLOPENFLAGS_INIT \
126-
.lock = { \
127-
.mutex = NULL, \
128-
.thread = PYTHREAD_INVALID_THREAD_ID, \
129-
.level = 0, \
130-
}, \
131122
.find_and_load = { \
132123
.header = 1, \
133124
}, \
@@ -180,11 +171,6 @@ extern void _PyImport_FiniCore(PyInterpreterState *interp);
180171
extern void _PyImport_FiniExternal(PyInterpreterState *interp);
181172

182173

183-
#ifdef HAVE_FORK
184-
extern PyStatus _PyImport_ReInitLock(PyInterpreterState *interp);
185-
#endif
186-
187-
188174
extern PyObject* _PyImport_GetBuiltinModuleNames(void);
189175

190176
struct _module_alias {

Include/internal/pycore_lock.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,18 @@ _PyOnceFlag_CallOnce(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg)
219219
return _PyOnceFlag_CallOnceSlow(flag, fn, arg);
220220
}
221221

222+
// A recursive mutex. The mutex should zero-initialized.
223+
typedef struct {
224+
PyMutex mutex;
225+
unsigned long long thread; // i.e., PyThread_get_thread_ident_ex()
226+
size_t level;
227+
} _PyRecursiveMutex;
228+
229+
PyAPI_FUNC(int) _PyRecursiveMutex_IsLockedByCurrentThread(_PyRecursiveMutex *m);
230+
PyAPI_FUNC(void) _PyRecursiveMutex_Lock(_PyRecursiveMutex *m);
231+
PyAPI_FUNC(void) _PyRecursiveMutex_Unlock(_PyRecursiveMutex *m);
232+
233+
222234
// A readers-writer (RW) lock. The lock supports multiple concurrent readers or
223235
// a single writer. The lock is write-preferring: if a writer is waiting while
224236
// the lock is read-locked then, new readers will be blocked. This avoids

Modules/_testinternalcapi/test_lock.c

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
#include "parts.h"
44
#include "pycore_lock.h"
5+
#include "pycore_pythread.h" // PyThread_get_thread_ident_ex()
56

67
#include "clinic/test_lock.c.h"
78

@@ -476,6 +477,29 @@ test_lock_rwlock(PyObject *self, PyObject *obj)
476477
Py_RETURN_NONE;
477478
}
478479

480+
static PyObject *
481+
test_lock_recursive(PyObject *self, PyObject *obj)
482+
{
483+
_PyRecursiveMutex m = (_PyRecursiveMutex){0};
484+
assert(!_PyRecursiveMutex_IsLockedByCurrentThread(&m));
485+
486+
_PyRecursiveMutex_Lock(&m);
487+
assert(m.thread == PyThread_get_thread_ident_ex());
488+
assert(PyMutex_IsLocked(&m.mutex));
489+
assert(m.level == 0);
490+
491+
_PyRecursiveMutex_Lock(&m);
492+
assert(m.level == 1);
493+
_PyRecursiveMutex_Unlock(&m);
494+
495+
_PyRecursiveMutex_Unlock(&m);
496+
assert(m.thread == 0);
497+
assert(!PyMutex_IsLocked(&m.mutex));
498+
assert(m.level == 0);
499+
500+
Py_RETURN_NONE;
501+
}
502+
479503
static PyMethodDef test_methods[] = {
480504
{"test_lock_basic", test_lock_basic, METH_NOARGS},
481505
{"test_lock_two_threads", test_lock_two_threads, METH_NOARGS},
@@ -485,6 +509,7 @@ static PyMethodDef test_methods[] = {
485509
{"test_lock_benchmark", test_lock_benchmark, METH_NOARGS},
486510
{"test_lock_once", test_lock_once, METH_NOARGS},
487511
{"test_lock_rwlock", test_lock_rwlock, METH_NOARGS},
512+
{"test_lock_recursive", test_lock_recursive, METH_NOARGS},
488513
{NULL, NULL} /* sentinel */
489514
};
490515

Modules/posixmodule.c

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
#include "pycore_call.h" // _PyObject_CallNoArgs()
1717
#include "pycore_ceval.h" // _PyEval_ReInitThreads()
1818
#include "pycore_fileutils.h" // _Py_closerange()
19-
#include "pycore_import.h" // _PyImport_ReInitLock()
2019
#include "pycore_initconfig.h" // _PyStatus_EXCEPTION()
2120
#include "pycore_long.h" // _PyLong_IsNegative()
2221
#include "pycore_moduleobject.h" // _PyModule_GetState()
@@ -627,10 +626,7 @@ PyOS_AfterFork_Parent(void)
627626
_PyEval_StartTheWorldAll(&_PyRuntime);
628627

629628
PyInterpreterState *interp = _PyInterpreterState_GET();
630-
if (_PyImport_ReleaseLock(interp) <= 0) {
631-
Py_FatalError("failed releasing import lock after fork");
632-
}
633-
629+
_PyImport_ReleaseLock(interp);
634630
run_at_forkers(interp->after_forkers_parent, 0);
635631
}
636632

@@ -675,10 +671,7 @@ PyOS_AfterFork_Child(void)
675671
_PyEval_StartTheWorldAll(&_PyRuntime);
676672
_PyThreadState_DeleteList(list);
677673

678-
status = _PyImport_ReInitLock(tstate->interp);
679-
if (_PyStatus_EXCEPTION(status)) {
680-
goto fatal_error;
681-
}
674+
_PyImport_ReleaseLock(tstate->interp);
682675

683676
_PySignal_AfterFork();
684677

Python/import.c

Lines changed: 7 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,7 @@ static struct _inittab *inittab_copy = NULL;
9494
(interp)->imports.import_func
9595

9696
#define IMPORT_LOCK(interp) \
97-
(interp)->imports.lock.mutex
98-
#define IMPORT_LOCK_THREAD(interp) \
99-
(interp)->imports.lock.thread
100-
#define IMPORT_LOCK_LEVEL(interp) \
101-
(interp)->imports.lock.level
97+
(interp)->imports.lock
10298

10399
#define FIND_AND_LOAD(interp) \
104100
(interp)->imports.find_and_load
@@ -115,74 +111,14 @@ static struct _inittab *inittab_copy = NULL;
115111
void
116112
_PyImport_AcquireLock(PyInterpreterState *interp)
117113
{
118-
unsigned long me = PyThread_get_thread_ident();
119-
if (me == PYTHREAD_INVALID_THREAD_ID)
120-
return; /* Too bad */
121-
if (IMPORT_LOCK(interp) == NULL) {
122-
IMPORT_LOCK(interp) = PyThread_allocate_lock();
123-
if (IMPORT_LOCK(interp) == NULL)
124-
return; /* Nothing much we can do. */
125-
}
126-
if (IMPORT_LOCK_THREAD(interp) == me) {
127-
IMPORT_LOCK_LEVEL(interp)++;
128-
return;
129-
}
130-
if (IMPORT_LOCK_THREAD(interp) != PYTHREAD_INVALID_THREAD_ID ||
131-
!PyThread_acquire_lock(IMPORT_LOCK(interp), 0))
132-
{
133-
PyThreadState *tstate = PyEval_SaveThread();
134-
PyThread_acquire_lock(IMPORT_LOCK(interp), WAIT_LOCK);
135-
PyEval_RestoreThread(tstate);
136-
}
137-
assert(IMPORT_LOCK_LEVEL(interp) == 0);
138-
IMPORT_LOCK_THREAD(interp) = me;
139-
IMPORT_LOCK_LEVEL(interp) = 1;
114+
_PyRecursiveMutex_Lock(&IMPORT_LOCK(interp));
140115
}
141116

142-
int
117+
void
143118
_PyImport_ReleaseLock(PyInterpreterState *interp)
144119
{
145-
unsigned long me = PyThread_get_thread_ident();
146-
if (me == PYTHREAD_INVALID_THREAD_ID || IMPORT_LOCK(interp) == NULL)
147-
return 0; /* Too bad */
148-
if (IMPORT_LOCK_THREAD(interp) != me)
149-
return -1;
150-
IMPORT_LOCK_LEVEL(interp)--;
151-
assert(IMPORT_LOCK_LEVEL(interp) >= 0);
152-
if (IMPORT_LOCK_LEVEL(interp) == 0) {
153-
IMPORT_LOCK_THREAD(interp) = PYTHREAD_INVALID_THREAD_ID;
154-
PyThread_release_lock(IMPORT_LOCK(interp));
155-
}
156-
return 1;
157-
}
158-
159-
#ifdef HAVE_FORK
160-
/* This function is called from PyOS_AfterFork_Child() to ensure that newly
161-
created child processes do not share locks with the parent.
162-
We now acquire the import lock around fork() calls but on some platforms
163-
(Solaris 9 and earlier? see isue7242) that still left us with problems. */
164-
PyStatus
165-
_PyImport_ReInitLock(PyInterpreterState *interp)
166-
{
167-
if (IMPORT_LOCK(interp) != NULL) {
168-
if (_PyThread_at_fork_reinit(&IMPORT_LOCK(interp)) < 0) {
169-
return _PyStatus_ERR("failed to create a new lock");
170-
}
171-
}
172-
173-
if (IMPORT_LOCK_LEVEL(interp) > 1) {
174-
/* Forked as a side effect of import */
175-
unsigned long me = PyThread_get_thread_ident();
176-
PyThread_acquire_lock(IMPORT_LOCK(interp), WAIT_LOCK);
177-
IMPORT_LOCK_THREAD(interp) = me;
178-
IMPORT_LOCK_LEVEL(interp)--;
179-
} else {
180-
IMPORT_LOCK_THREAD(interp) = PYTHREAD_INVALID_THREAD_ID;
181-
IMPORT_LOCK_LEVEL(interp) = 0;
182-
}
183-
return _PyStatus_OK();
120+
_PyRecursiveMutex_Unlock(&IMPORT_LOCK(interp));
184121
}
185-
#endif
186122

187123

188124
/***************/
@@ -4111,11 +4047,6 @@ _PyImport_FiniCore(PyInterpreterState *interp)
41114047
PyErr_FormatUnraisable("Exception ignored on clearing sys.modules");
41124048
}
41134049

4114-
if (IMPORT_LOCK(interp) != NULL) {
4115-
PyThread_free_lock(IMPORT_LOCK(interp));
4116-
IMPORT_LOCK(interp) = NULL;
4117-
}
4118-
41194050
_PyImport_ClearCore(interp);
41204051
}
41214052

@@ -4248,8 +4179,7 @@ _imp_lock_held_impl(PyObject *module)
42484179
/*[clinic end generated code: output=8b89384b5e1963fc input=9b088f9b217d9bdf]*/
42494180
{
42504181
PyInterpreterState *interp = _PyInterpreterState_GET();
4251-
return PyBool_FromLong(
4252-
IMPORT_LOCK_THREAD(interp) != PYTHREAD_INVALID_THREAD_ID);
4182+
return PyBool_FromLong(PyMutex_IsLocked(&IMPORT_LOCK(interp).mutex));
42534183
}
42544184

42554185
/*[clinic input]
@@ -4283,11 +4213,12 @@ _imp_release_lock_impl(PyObject *module)
42834213
/*[clinic end generated code: output=7faab6d0be178b0a input=934fb11516dd778b]*/
42844214
{
42854215
PyInterpreterState *interp = _PyInterpreterState_GET();
4286-
if (_PyImport_ReleaseLock(interp) < 0) {
4216+
if (!_PyRecursiveMutex_IsLockedByCurrentThread(&IMPORT_LOCK(interp))) {
42874217
PyErr_SetString(PyExc_RuntimeError,
42884218
"not holding the import lock");
42894219
return NULL;
42904220
}
4221+
_PyImport_ReleaseLock(interp);
42914222
Py_RETURN_NONE;
42924223
}
42934224

Python/lock.c

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,48 @@ _PyOnceFlag_CallOnceSlow(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg)
366366
}
367367
}
368368

369+
static int
370+
recursive_mutex_is_owned_by(_PyRecursiveMutex *m, PyThread_ident_t tid)
371+
{
372+
return _Py_atomic_load_ullong_relaxed(&m->thread) == tid;
373+
}
374+
375+
int
376+
_PyRecursiveMutex_IsLockedByCurrentThread(_PyRecursiveMutex *m)
377+
{
378+
return recursive_mutex_is_owned_by(m, PyThread_get_thread_ident_ex());
379+
}
380+
381+
void
382+
_PyRecursiveMutex_Lock(_PyRecursiveMutex *m)
383+
{
384+
PyThread_ident_t thread = PyThread_get_thread_ident_ex();
385+
if (recursive_mutex_is_owned_by(m, thread)) {
386+
m->level++;
387+
return;
388+
}
389+
PyMutex_Lock(&m->mutex);
390+
_Py_atomic_store_ullong_relaxed(&m->thread, thread);
391+
assert(m->level == 0);
392+
}
393+
394+
void
395+
_PyRecursiveMutex_Unlock(_PyRecursiveMutex *m)
396+
{
397+
PyThread_ident_t thread = PyThread_get_thread_ident_ex();
398+
if (!recursive_mutex_is_owned_by(m, thread)) {
399+
Py_FatalError("unlocking a recursive mutex that is not owned by the"
400+
" current thread");
401+
}
402+
if (m->level > 0) {
403+
m->level--;
404+
return;
405+
}
406+
assert(m->level == 0);
407+
_Py_atomic_store_ullong_relaxed(&m->thread, 0);
408+
PyMutex_Unlock(&m->mutex);
409+
}
410+
369411
#define _Py_WRITE_LOCKED 1
370412
#define _PyRWMutex_READER_SHIFT 2
371413
#define _Py_RWMUTEX_MAX_READERS (UINTPTR_MAX >> _PyRWMutex_READER_SHIFT)

Tools/tsan/suppressions_free_threading.txt

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,6 @@ race:free_threadstate
2626
race_top:_add_to_weak_set
2727
race_top:_in_weak_set
2828
race_top:_PyEval_EvalFrameDefault
29-
race_top:_PyImport_AcquireLock
30-
race_top:_PyImport_ReleaseLock
3129
race_top:_PyType_HasFeature
3230
race_top:assign_version_tag
3331
race_top:insertdict
@@ -41,9 +39,7 @@ race_top:set_discard_entry
4139
race_top:set_inheritable
4240
race_top:Py_SET_TYPE
4341
race_top:_PyDict_CheckConsistency
44-
race_top:_PyImport_AcquireLock
4542
race_top:_Py_dict_lookup_threadsafe
46-
race_top:_imp_release_lock
4743
race_top:_multiprocessing_SemLock_acquire_impl
4844
race_top:dictiter_new
4945
race_top:dictresize

0 commit comments

Comments
 (0)