Skip to content

Commit 5722416

Browse files
committed
_threadmodule: thread-safety fixes
- Make rlock thread-safe - Use atomics to modify interp->num_threads
1 parent 3cfbc49 commit 5722416

File tree

2 files changed

+31
-28
lines changed

2 files changed

+31
-28
lines changed

Include/internal/pycore_interp.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ struct _is {
9292
/* The linked list of threads, newest first. */
9393
PyThreadState *head;
9494
/* Used in Modules/_threadmodule.c. */
95-
long count;
95+
Py_ssize_t count;
9696
/* Support for runtime thread stack size tuning.
9797
A value of 0 means using the platform's default stack size
9898
or the size specified by the THREAD_STACK_SIZE macro. */

Modules/_threadmodule.c

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ static PyType_Spec lock_type_spec = {
319319
typedef struct {
320320
PyObject_HEAD
321321
PyThread_type_lock rlock_lock;
322-
unsigned long rlock_owner;
322+
uintptr_t rlock_owner;
323323
unsigned long rlock_count;
324324
PyObject *in_weakreflist;
325325
} rlockobject;
@@ -331,7 +331,6 @@ rlock_traverse(rlockobject *self, visitproc visit, void *arg)
331331
return 0;
332332
}
333333

334-
335334
static void
336335
rlock_dealloc(rlockobject *self)
337336
{
@@ -352,18 +351,24 @@ rlock_dealloc(rlockobject *self)
352351
Py_DECREF(tp);
353352
}
354353

354+
static int
355+
rlock_is_owned(rlockobject *self)
356+
{
357+
uintptr_t tid = _Py_ThreadId();
358+
uintptr_t owner_tid = _Py_atomic_load_uintptr_relaxed(&self->rlock_owner);
359+
return owner_tid == tid && self->rlock_count > 0;
360+
}
361+
355362
static PyObject *
356363
rlock_acquire(rlockobject *self, PyObject *args, PyObject *kwds)
357364
{
358365
_PyTime_t timeout;
359-
unsigned long tid;
360366
PyLockStatus r = PY_LOCK_ACQUIRED;
361367

362368
if (lock_acquire_parse_args(args, kwds, &timeout) < 0)
363369
return NULL;
364370

365-
tid = PyThread_get_thread_ident();
366-
if (self->rlock_count > 0 && tid == self->rlock_owner) {
371+
if (rlock_is_owned(self)) {
367372
unsigned long count = self->rlock_count + 1;
368373
if (count <= self->rlock_count) {
369374
PyErr_SetString(PyExc_OverflowError,
@@ -376,7 +381,7 @@ rlock_acquire(rlockobject *self, PyObject *args, PyObject *kwds)
376381
r = acquire_timed(self->rlock_lock, timeout);
377382
if (r == PY_LOCK_ACQUIRED) {
378383
assert(self->rlock_count == 0);
379-
self->rlock_owner = tid;
384+
_Py_atomic_store_uintptr_relaxed(&self->rlock_owner, _Py_ThreadId());
380385
self->rlock_count = 1;
381386
}
382387
else if (r == PY_LOCK_INTR) {
@@ -405,15 +410,13 @@ the lock is taken and its internal counter initialized to 1.");
405410
static PyObject *
406411
rlock_release(rlockobject *self, PyObject *Py_UNUSED(ignored))
407412
{
408-
unsigned long tid = PyThread_get_thread_ident();
409-
410-
if (self->rlock_count == 0 || self->rlock_owner != tid) {
413+
if (!rlock_is_owned(self)) {
411414
PyErr_SetString(PyExc_RuntimeError,
412-
"cannot release un-acquired lock");
415+
"cannot release un-acquired lock1");
413416
return NULL;
414417
}
415418
if (--self->rlock_count == 0) {
416-
self->rlock_owner = 0;
419+
_Py_atomic_store_uintptr_relaxed(&self->rlock_owner, 0);
417420
PyThread_release_lock(self->rlock_lock);
418421
}
419422
Py_RETURN_NONE;
@@ -434,11 +437,11 @@ to be available for other threads.");
434437
static PyObject *
435438
rlock_acquire_restore(rlockobject *self, PyObject *args)
436439
{
437-
unsigned long owner;
440+
unsigned long long owner;
438441
unsigned long count;
439442
int r = 1;
440443

441-
if (!PyArg_ParseTuple(args, "(kk):_acquire_restore", &count, &owner))
444+
if (!PyArg_ParseTuple(args, "(kK):_acquire_restore", &count, &owner))
442445
return NULL;
443446

444447
if (!PyThread_acquire_lock(self->rlock_lock, 0)) {
@@ -451,7 +454,7 @@ rlock_acquire_restore(rlockobject *self, PyObject *args)
451454
return NULL;
452455
}
453456
assert(self->rlock_count == 0);
454-
self->rlock_owner = owner;
457+
_Py_atomic_store_uintptr_relaxed(&self->rlock_owner, (uintptr_t)owner);
455458
self->rlock_count = count;
456459
Py_RETURN_NONE;
457460
}
@@ -464,7 +467,7 @@ For internal use by `threading.Condition`.");
464467
static PyObject *
465468
rlock_release_save(rlockobject *self, PyObject *Py_UNUSED(ignored))
466469
{
467-
unsigned long owner;
470+
uintptr_t owner;
468471
unsigned long count;
469472

470473
if (self->rlock_count == 0) {
@@ -476,9 +479,9 @@ rlock_release_save(rlockobject *self, PyObject *Py_UNUSED(ignored))
476479
owner = self->rlock_owner;
477480
count = self->rlock_count;
478481
self->rlock_count = 0;
479-
self->rlock_owner = 0;
482+
_Py_atomic_store_uintptr_relaxed(&self->rlock_owner, 0);
480483
PyThread_release_lock(self->rlock_lock);
481-
return Py_BuildValue("kk", count, owner);
484+
return Py_BuildValue("kK", count, (unsigned long long)owner);
482485
}
483486

484487
PyDoc_STRVAR(rlock_release_save_doc,
@@ -488,11 +491,9 @@ For internal use by `threading.Condition`.");
488491

489492

490493
static PyObject *
491-
rlock_is_owned(rlockobject *self, PyObject *Py_UNUSED(ignored))
494+
rlock__is_owned(rlockobject *self, PyObject *Py_UNUSED(ignored))
492495
{
493-
unsigned long tid = PyThread_get_thread_ident();
494-
495-
if (self->rlock_count > 0 && self->rlock_owner == tid) {
496+
if (rlock_is_owned(self)) {
496497
Py_RETURN_TRUE;
497498
}
498499
Py_RETURN_FALSE;
@@ -526,9 +527,11 @@ rlock_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
526527
static PyObject *
527528
rlock_repr(rlockobject *self)
528529
{
529-
return PyUnicode_FromFormat("<%s %s object owner=%ld count=%lu at %p>",
530+
uintptr_t owner = _Py_atomic_load_uintptr_relaxed(&self->rlock_owner);
531+
532+
return PyUnicode_FromFormat("<%s %s object owner=%p count=%llu at %p>",
530533
self->rlock_count ? "locked" : "unlocked",
531-
Py_TYPE(self)->tp_name, self->rlock_owner,
534+
Py_TYPE(self)->tp_name, (const void *)owner,
532535
self->rlock_count, self);
533536
}
534537

@@ -555,7 +558,7 @@ static PyMethodDef rlock_methods[] = {
555558
METH_VARARGS | METH_KEYWORDS, rlock_acquire_doc},
556559
{"release", (PyCFunction)rlock_release,
557560
METH_NOARGS, rlock_release_doc},
558-
{"_is_owned", (PyCFunction)rlock_is_owned,
561+
{"_is_owned", (PyCFunction)rlock__is_owned,
559562
METH_NOARGS, rlock_is_owned_doc},
560563
{"_acquire_restore", (PyCFunction)rlock_acquire_restore,
561564
METH_VARARGS, rlock_acquire_restore_doc},
@@ -1272,7 +1275,7 @@ thread_run(void *boot_raw)
12721275
#endif
12731276
_PyThreadState_SetCurrent(tstate);
12741277
PyEval_AcquireThread(tstate);
1275-
tstate->interp->threads.count++;
1278+
_Py_atomic_add_ssize(&tstate->interp->threads.count, 1);
12761279

12771280
PyObject *res = PyObject_Call(boot->func, boot->args, boot->kwargs);
12781281
if (res == NULL) {
@@ -1288,7 +1291,7 @@ thread_run(void *boot_raw)
12881291
}
12891292

12901293
thread_bootstate_free(boot);
1291-
tstate->interp->threads.count--;
1294+
_Py_atomic_add_ssize(&tstate->interp->threads.count, -1);
12921295
PyThreadState_Clear(tstate);
12931296
_PyThreadState_DeleteCurrent(tstate);
12941297

@@ -1531,7 +1534,7 @@ static PyObject *
15311534
thread__count(PyObject *self, PyObject *Py_UNUSED(ignored))
15321535
{
15331536
PyInterpreterState *interp = _PyInterpreterState_GET();
1534-
return PyLong_FromLong(interp->threads.count);
1537+
return PyLong_FromSize_t(_Py_atomic_load_ssize(&interp->threads.count));
15351538
}
15361539

15371540
PyDoc_STRVAR(_count_doc,

0 commit comments

Comments
 (0)