gh-120973: Fix thread-safety issues with `threading.local` (#121655)

This is a small refactoring to the current design that allows us to avoid manually iterating over threads. This should also fix gh-118490.
author: mpage <mpage@meta.com> 2024-07-19 17:22:02 (GMT)
committer: GitHub <noreply@github.com> 2024-07-19 17:22:02 (GMT)
commit: e059aa6b01030310125477c3ed1da0491020fe10 (patch)
tree: c62b22c966cfa6cdd0d53779d3e9ad965713bf2e /Modules
parent: 2009e25e26040dca32696e70f91f13665350e7fd (diff)
download: cpython-e059aa6b01030310125477c3ed1da0491020fe10.zip
cpython-e059aa6b01030310125477c3ed1da0491020fe10.tar.gz
cpython-e059aa6b01030310125477c3ed1da0491020fe10.tar.bz2
1 files changed, 234 insertions, 150 deletions
diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c
index 39d3097..d21a37d 100644
--- a/Modules/_threadmodule.c
+++ b/Modules/_threadmodule.c
@@ -1350,33 +1350,44 @@ newlockobject(PyObject *module)
    Our implementation uses small "localdummy" objects in order to break
    the reference chain. These trivial objects are hashable (using the
    default scheme of identity hashing) and weakrefable.
-   Each thread-state holds a separate localdummy for each local object
-   (as a /strong reference/),
-   and each thread-local object holds a dict mapping /weak references/
-   of localdummies to local dicts.
+
+   Each thread-state holds two separate localdummy objects:
+
+   - `threading_local_key` is used as a key to retrieve the locals dictionary
+     for the thread in any `threading.local` object.
+   - `threading_local_sentinel` is used to signal when a thread is being
+     destroyed. Consequently, the associated thread-state must hold the only
+     reference.
+
+   Each `threading.local` object contains a dict mapping localdummy keys to
+   locals dicts and a set containing weak references to localdummy
+   sentinels. Each sentinel weak reference has a callback that removes itself
+   and the locals dict for the key from the `threading.local` object when
+   called.
 
    Therefore:
-   - only the thread-state dict holds a strong reference to the dummies
-   - only the thread-local object holds a strong reference to the local dicts
-   - only outside objects (application- or library-level) hold strong
-     references to the thread-local objects
-   - as soon as a thread-state dict is destroyed, the weakref callbacks of all
-     dummies attached to that thread are called, and destroy the corresponding
-     local dicts from thread-local objects
-   - as soon as a thread-local object is destroyed, its local dicts are
-     destroyed and its dummies are manually removed from all thread states
-   - the GC can do its work correctly when a thread-local object is dangling,
-     without any interference from the thread-state dicts
-
-   As an additional optimization, each localdummy holds a borrowed reference
-   to the corresponding localdict.  This borrowed reference is only used
-   by the thread-local object which has created the localdummy, which should
-   guarantee that the localdict still exists when accessed.
+   - The thread-state only holds strong references to localdummy objects, which
+     cannot participate in cycles.
+   - Only outside objects (application- or library-level) hold strong
+     references to the thread-local objects.
+   - As soon as thread-state's sentinel dummy is destroyed the callbacks for
+     all weakrefs attached to the sentinel are called, and destroy the
+     corresponding local dicts from thread-local objects.
+   - As soon as a thread-local object is destroyed, its local dicts are
+     destroyed.
+   - The GC can do its work correctly when a thread-local object is dangling,
+     without any interference from the thread-state dicts.
+
+   This dual key arrangement is necessary to ensure that `threading.local`
+   values can be retrieved from finalizers. If we were to only keep a mapping
+   of localdummy weakrefs to locals dicts it's possible that the weakrefs would
+   be cleared before finalizers were called (GC currently clears weakrefs that
+   are garbage before invoking finalizers), causing lookups in finalizers to
+   fail.
 */
 
 typedef struct {
     PyObject_HEAD
-    PyObject *localdict;        /* Borrowed reference! */
     PyObject *weakreflist;      /* List of weak references to self */
 } localdummyobject;
 
@@ -1413,80 +1424,60 @@ static PyType_Spec local_dummy_type_spec = {
 
 typedef struct {
     PyObject_HEAD
-    PyObject *key;
     PyObject *args;
     PyObject *kw;
     PyObject *weakreflist;      /* List of weak references to self */
-    /* A {localdummy weakref -> localdict} dict */
-    PyObject *dummies;
-    /* The callback for weakrefs to localdummies */
-    PyObject *wr_callback;
+    /* A {localdummy -> localdict} dict */
+    PyObject *localdicts;
+    /* A set of weakrefs to thread sentinels localdummies*/
+    PyObject *thread_watchdogs;
 } localobject;
 
 /* Forward declaration */
-static PyObject *_ldict(localobject *self, thread_module_state *state);
-static PyObject *_localdummy_destroyed(PyObject *meth_self, PyObject *dummyweakref);
+static int create_localsdict(localobject *self, thread_module_state *state,
+                             PyObject **localsdict, PyObject **sentinel_wr);
+static PyObject *clear_locals(PyObject *meth_self, PyObject *dummyweakref);
 
-/* Create and register the dummy for the current thread.
-   Returns a borrowed reference of the corresponding local dict */
+/* Create a weakref to the sentinel localdummy for the current thread */
 static PyObject *
-_local_create_dummy(localobject *self, thread_module_state *state)
+create_sentinel_wr(localobject *self)
 {
-    PyObject *ldict = NULL, *wr = NULL;
-    localdummyobject *dummy = NULL;
-    PyTypeObject *type = state->local_dummy_type;
+    static PyMethodDef wr_callback_def = {
+        "clear_locals", (PyCFunction) clear_locals, METH_O
+    };
 
-    PyObject *tdict = PyThreadState_GetDict();
-    if (tdict == NULL) {
-        PyErr_SetString(PyExc_SystemError,
-                        "Couldn't get thread-state dictionary");
-        goto err;
-    }
+    PyThreadState *tstate = PyThreadState_Get();
 
-    ldict = PyDict_New();
-    if (ldict == NULL) {
-        goto err;
-    }
-    dummy = (localdummyobject *) type->tp_alloc(type, 0);
-    if (dummy == NULL) {
-        goto err;
-    }
-    dummy->localdict = ldict;
-    wr = PyWeakref_NewRef((PyObject *) dummy, self->wr_callback);
-    if (wr == NULL) {
-        goto err;
+    /* We use a weak reference to self in the callback closure
+       in order to avoid spurious reference cycles */
+    PyObject *self_wr = PyWeakref_NewRef((PyObject *) self, NULL);
+    if (self_wr == NULL) {
+        return NULL;
     }
 
-    /* As a side-effect, this will cache the weakref's hash before the
-       dummy gets deleted */
-    int r = PyDict_SetItem(self->dummies, wr, ldict);
-    if (r < 0) {
-        goto err;
+    PyObject *args = PyTuple_New(2);
+    if (args == NULL) {
+        Py_DECREF(self_wr);
+        return NULL;
     }
-    Py_CLEAR(wr);
-    r = PyDict_SetItem(tdict, self->key, (PyObject *) dummy);
-    if (r < 0) {
-        goto err;
+    PyTuple_SET_ITEM(args, 0, self_wr);
+    PyTuple_SET_ITEM(args, 1, Py_NewRef(tstate->threading_local_key));
+
+    PyObject *cb = PyCFunction_New(&wr_callback_def, args);
+    Py_DECREF(args);
+    if (cb == NULL) {
+        return NULL;
     }
-    Py_CLEAR(dummy);
 
-    Py_DECREF(ldict);
-    return ldict;
+    PyObject *wr = PyWeakref_NewRef(tstate->threading_local_sentinel, cb);
+    Py_DECREF(cb);
 
-err:
-    Py_XDECREF(ldict);
-    Py_XDECREF(wr);
-    Py_XDECREF(dummy);
-    return NULL;
+    return wr;
 }
 
 static PyObject *
 local_new(PyTypeObject *type, PyObject *args, PyObject *kw)
 {
-    static PyMethodDef wr_callback_def = {
-        "_localdummy_destroyed", (PyCFunction) _localdummy_destroyed, METH_O
-    };
-
     if (type->tp_init == PyBaseObject_Type.tp_init) {
         int rc = 0;
         if (args != NULL)
@@ -1513,30 +1504,25 @@ local_new(PyTypeObject *type, PyObject *args, PyObject *kw)
 
     self->args = Py_XNewRef(args);
     self->kw = Py_XNewRef(kw);
-    self->key = PyUnicode_FromFormat("thread.local.%p", self);
-    if (self->key == NULL) {
-        goto err;
-    }
 
-    self->dummies = PyDict_New();
-    if (self->dummies == NULL) {
+    self->localdicts = PyDict_New();
+    if (self->localdicts == NULL) {
         goto err;
     }
 
-    /* We use a weak reference to self in the callback closure
-       in order to avoid spurious reference cycles */
-    PyObject *wr = PyWeakref_NewRef((PyObject *) self, NULL);
-    if (wr == NULL) {
-        goto err;
-    }
-    self->wr_callback = PyCFunction_NewEx(&wr_callback_def, wr, NULL);
-    Py_DECREF(wr);
-    if (self->wr_callback == NULL) {
+    self->thread_watchdogs = PySet_New(NULL);
+    if (self->thread_watchdogs == NULL) {
         goto err;
     }
-    if (_local_create_dummy(self, state) == NULL) {
+
+    PyObject *localsdict = NULL;
+    PyObject *sentinel_wr = NULL;
+    if (create_localsdict(self, state, &localsdict, &sentinel_wr) < 0) {
         goto err;
     }
+    Py_DECREF(localsdict);
+    Py_DECREF(sentinel_wr);
+
     return (PyObject *)self;
 
   err:
@@ -1550,7 +1536,8 @@ local_traverse(localobject *self, visitproc visit, void *arg)
     Py_VISIT(Py_TYPE(self));
     Py_VISIT(self->args);
     Py_VISIT(self->kw);
-    Py_VISIT(self->dummies);
+    Py_VISIT(self->localdicts);
+    Py_VISIT(self->thread_watchdogs);
     return 0;
 }
 
@@ -1559,27 +1546,8 @@ local_clear(localobject *self)
 {
     Py_CLEAR(self->args);
     Py_CLEAR(self->kw);
-    Py_CLEAR(self->dummies);
-    Py_CLEAR(self->wr_callback);
-    /* Remove all strong references to dummies from the thread states */
-    if (self->key) {
-        PyInterpreterState *interp = _PyInterpreterState_GET();
-        _PyRuntimeState *runtime = &_PyRuntime;
-        HEAD_LOCK(runtime);
-        PyThreadState *tstate = PyInterpreterState_ThreadHead(interp);
-        HEAD_UNLOCK(runtime);
-        while (tstate) {
-            if (tstate->dict) {
-                if (PyDict_Pop(tstate->dict, self->key, NULL) < 0) {
-                    // Silently ignore error
-                    PyErr_Clear();
-                }
-            }
-            HEAD_LOCK(runtime);
-            tstate = PyThreadState_Next(tstate);
-            HEAD_UNLOCK(runtime);
-        }
-    }
+    Py_CLEAR(self->localdicts);
+    Py_CLEAR(self->thread_watchdogs);
     return 0;
 }
 
@@ -1595,48 +1563,142 @@ local_dealloc(localobject *self)
     PyObject_GC_UnTrack(self);
 
     local_clear(self);
-    Py_XDECREF(self->key);
 
     PyTypeObject *tp = Py_TYPE(self);
     tp->tp_free((PyObject*)self);
     Py_DECREF(tp);
 }
 
-/* Returns a borrowed reference to the local dict, creating it if necessary */
+/* Create the TLS key and sentinel if they don't exist */
+static int
+create_localdummies(thread_module_state *state)
+{
+    PyThreadState *tstate = _PyThreadState_GET();
+
+    if (tstate->threading_local_key != NULL) {
+        return 0;
+    }
+
+    PyTypeObject *ld_type = state->local_dummy_type;
+    tstate->threading_local_key = ld_type->tp_alloc(ld_type, 0);
+    if (tstate->threading_local_key == NULL) {
+        return -1;
+    }
+
+    tstate->threading_local_sentinel = ld_type->tp_alloc(ld_type, 0);
+    if (tstate->threading_local_sentinel == NULL) {
+        Py_CLEAR(tstate->threading_local_key);
+        return -1;
+    }
+
+    return 0;
+}
+
+/* Insert a localsdict and sentinel weakref for the current thread, placing
+   strong references in localsdict and sentinel_wr, respectively.
+*/
+static int
+create_localsdict(localobject *self, thread_module_state *state,
+                  PyObject **localsdict, PyObject **sentinel_wr)
+{
+    PyThreadState *tstate = _PyThreadState_GET();
+    PyObject *ldict = NULL;
+    PyObject *wr = NULL;
+
+    if (create_localdummies(state) < 0) {
+        goto err;
+    }
+
+    /* Create and insert the locals dict and sentinel weakref */
+    ldict = PyDict_New();
+    if (ldict == NULL) {
+        goto err;
+    }
+
+    if (PyDict_SetItem(self->localdicts, tstate->threading_local_key, ldict) <
+        0) {
+        goto err;
+    }
+
+    wr = create_sentinel_wr(self);
+    if (wr == NULL) {
+        PyObject *exc = PyErr_GetRaisedException();
+        if (PyDict_DelItem(self->localdicts, tstate->threading_local_key) <
+            0) {
+            PyErr_WriteUnraisable((PyObject *)self);
+        }
+        PyErr_SetRaisedException(exc);
+        goto err;
+    }
+
+    if (PySet_Add(self->thread_watchdogs, wr) < 0) {
+        PyObject *exc = PyErr_GetRaisedException();
+        if (PyDict_DelItem(self->localdicts, tstate->threading_local_key) <
+            0) {
+            PyErr_WriteUnraisable((PyObject *)self);
+        }
+        PyErr_SetRaisedException(exc);
+        goto err;
+    }
+
+    *localsdict = ldict;
+    *sentinel_wr = wr;
+    return 0;
+
+err:
+    Py_XDECREF(ldict);
+    Py_XDECREF(wr);
+    return -1;
+}
+
+/* Return a strong reference to the locals dict for the current thread,
+   creating it if necessary.
+*/
 static PyObject *
 _ldict(localobject *self, thread_module_state *state)
 {
-    PyObject *tdict = PyThreadState_GetDict();
-    if (tdict == NULL) {
-        PyErr_SetString(PyExc_SystemError,
-                        "Couldn't get thread-state dictionary");
+    if (create_localdummies(state) < 0) {
         return NULL;
     }
 
+    /* Check if a localsdict already exists */
     PyObject *ldict;
-    PyObject *dummy = PyDict_GetItemWithError(tdict, self->key);
-    if (dummy == NULL) {
-        if (PyErr_Occurred()) {
-            return NULL;
-        }
-        ldict = _local_create_dummy(self, state);
-        if (ldict == NULL)
-            return NULL;
+    PyThreadState *tstate = _PyThreadState_GET();
+    if (PyDict_GetItemRef(self->localdicts, tstate->threading_local_key,
+                          &ldict) < 0) {
+        return NULL;
+    }
+    if (ldict != NULL) {
+        return ldict;
+    }
 
-        if (Py_TYPE(self)->tp_init != PyBaseObject_Type.tp_init &&
-            Py_TYPE(self)->tp_init((PyObject*)self,
-                                   self->args, self->kw) < 0) {
-            /* we need to get rid of ldict from thread so
-               we create a new one the next time we do an attr
-               access */
-            PyDict_DelItem(tdict, self->key);
-            return NULL;
-        }
+    /* threading.local hasn't been instantiated for this thread */
+    PyObject *wr;
+    if (create_localsdict(self, state, &ldict, &wr) < 0) {
+        return NULL;
     }
-    else {
-        assert(Py_IS_TYPE(dummy, state->local_dummy_type));
-        ldict = ((localdummyobject *) dummy)->localdict;
+
+    /* run __init__ if we're a subtype of `threading.local` */
+    if (Py_TYPE(self)->tp_init != PyBaseObject_Type.tp_init &&
+        Py_TYPE(self)->tp_init((PyObject *)self, self->args, self->kw) < 0) {
+        /* we need to get rid of ldict from thread so
+           we create a new one the next time we do an attr
+           access */
+        PyObject *exc = PyErr_GetRaisedException();
+        if (PyDict_DelItem(self->localdicts, tstate->threading_local_key) <
+            0) {
+            PyErr_WriteUnraisable((PyObject *)self);
+            PyErr_Clear();
+        }
+        if (PySet_Discard(self->thread_watchdogs, wr) < 0) {
+            PyErr_WriteUnraisable((PyObject *)self);
+        }
+        PyErr_SetRaisedException(exc);
+        Py_DECREF(ldict);
+        Py_DECREF(wr);
+        return NULL;
     }
+    Py_DECREF(wr);
 
     return ldict;
 }
@@ -1650,21 +1712,28 @@ local_setattro(localobject *self, PyObject *name, PyObject *v)
 
     PyObject *ldict = _ldict(self, state);
     if (ldict == NULL) {
-        return -1;
+        goto err;
     }
 
     int r = PyObject_RichCompareBool(name, &_Py_ID(__dict__), Py_EQ);
     if (r == -1) {
-        return -1;
+        goto err;
     }
     if (r == 1) {
         PyErr_Format(PyExc_AttributeError,
                      "'%.100s' object attribute '%U' is read-only",
                      Py_TYPE(self)->tp_name, name);
-        return -1;
+        goto err;
     }
 
-    return _PyObject_GenericSetAttrWithDict((PyObject *)self, name, v, ldict);
+    int st =
+        _PyObject_GenericSetAttrWithDict((PyObject *)self, name, v, ldict);
+    Py_DECREF(ldict);
+    return st;
+
+err:
+    Py_XDECREF(ldict);
+    return -1;
 }
 
 static PyObject *local_getattro(localobject *, PyObject *);
@@ -1707,34 +1776,42 @@ local_getattro(localobject *self, PyObject *name)
 
     int r = PyObject_RichCompareBool(name, &_Py_ID(__dict__), Py_EQ);
     if (r == 1) {
-        return Py_NewRef(ldict);
+        return ldict;
     }
     if (r == -1) {
+        Py_DECREF(ldict);
         return NULL;
     }
 
     if (!Py_IS_TYPE(self, state->local_type)) {
         /* use generic lookup for subtypes */
-        return _PyObject_GenericGetAttrWithDict((PyObject *)self, name,
-                                                ldict, 0);
+        PyObject *res =
+            _PyObject_GenericGetAttrWithDict((PyObject *)self, name, ldict, 0);
+        Py_DECREF(ldict);
+        return res;
     }
 
     /* Optimization: just look in dict ourselves */
     PyObject *value;
     if (PyDict_GetItemRef(ldict, name, &value) != 0) {
         // found or error
+        Py_DECREF(ldict);
         return value;
     }
 
     /* Fall back on generic to get __class__ and __dict__ */
-    return _PyObject_GenericGetAttrWithDict(
-        (PyObject *)self, name, ldict, 0);
+    PyObject *res =
+        _PyObject_GenericGetAttrWithDict((PyObject *)self, name, ldict, 0);
+    Py_DECREF(ldict);
+    return res;
 }
 
-/* Called when a dummy is destroyed. */
+/* Called when a dummy is destroyed, indicating that the owning thread is being
+ * cleared. */
 static PyObject *
-_localdummy_destroyed(PyObject *localweakref, PyObject *dummyweakref)
+clear_locals(PyObject *locals_and_key, PyObject *dummyweakref)
 {
+    PyObject *localweakref = PyTuple_GetItem(locals_and_key, 0);
     localobject *self = (localobject *)_PyWeakref_GET_REF(localweakref);
     if (self == NULL) {
         Py_RETURN_NONE;
@@ -1742,11 +1819,18 @@ _localdummy_destroyed(PyObject *localweakref, PyObject *dummyweakref)
 
     /* If the thread-local object is still alive and not being cleared,
        remove the corresponding local dict */
-    if (self->dummies != NULL) {
-        if (PyDict_Pop(self->dummies, dummyweakref, NULL) < 0) {
+    if (self->localdicts != NULL) {
+        PyObject *key = PyTuple_GetItem(locals_and_key, 1);
+        if (PyDict_Pop(self->localdicts, key, NULL) < 0) {
             PyErr_WriteUnraisable((PyObject*)self);
         }
     }
+    if (self->thread_watchdogs != NULL) {
+        if (PySet_Discard(self->thread_watchdogs, dummyweakref) < 0) {
+            PyErr_WriteUnraisable((PyObject *)self);
+        }
+    }
+
     Py_DECREF(self);
     Py_RETURN_NONE;
 }
author	mpage <mpage@meta.com>	2024-07-19 17:22:02 (GMT)
committer	GitHub <noreply@github.com>	2024-07-19 17:22:02 (GMT)
commit	e059aa6b01030310125477c3ed1da0491020fe10 (patch)
tree	c62b22c966cfa6cdd0d53779d3e9ad965713bf2e /Modules
parent	2009e25e26040dca32696e70f91f13665350e7fd (diff)
download	cpython-e059aa6b01030310125477c3ed1da0491020fe10.zip cpython-e059aa6b01030310125477c3ed1da0491020fe10.tar.gz cpython-e059aa6b01030310125477c3ed1da0491020fe10.tar.bz2