[3.9] GH-100892: Fix race in clearing threading.local (GH-100922) (#100939)

kumaraditya303 · web-flow · commit 6954203c9fb7 · 2023-01-20T23:21:40.000+01:00
[3.9] [3.10] GH-100892: Fix race in clearing `threading.local` (GH-100922). (cherry picked from commit 762745a) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>. (cherry picked from commit 683e9fe) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
diff --git a/Lib/test/test_threading_local.py b/Lib/test/test_threading_local.py
@@ -193,6 +193,22 @@ class X:
         self.assertIsNone(wr())
 
 
+    def test_threading_local_clear_race(self):
+        # See https://github.com/python/cpython/issues/100892
+
+        try:
+            import _testcapi
+        except ImportError:
+            unittest.skip("requires _testcapi")
+
+        _testcapi.call_in_temporary_c_thread(lambda: None, False)
+
+        for _ in range(1000):
+            _ = threading.local()
+
+        _testcapi.join_temporary_c_thread()
+
+
 class ThreadLocalTest(unittest.TestCase, BaseLocalTest):
     _local = _thread._local
 
diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-01-10-14-11-17.gh-issue-100892.qfBVYI.rst b/Misc/NEWS.d/next/Core and Builtins/2023-01-10-14-11-17.gh-issue-100892.qfBVYI.rst
@@ -0,0 +1 @@
+Fix race while iterating over thread states in clearing :class:`threading.local`. Patch by Kumar Aditya.
diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
@@ -4239,12 +4239,19 @@ temporary_c_thread(void *data)
     PyThread_exit_thread();
 }
 
+static test_c_thread_t test_c_thread;
+
 static PyObject *
-call_in_temporary_c_thread(PyObject *self, PyObject *callback)
+call_in_temporary_c_thread(PyObject *self, PyObject *args)
 {
     PyObject *res = NULL;
-    test_c_thread_t test_c_thread;
+    PyObject *callback = NULL;
     long thread;
+    int wait = 1;
+    if (!PyArg_ParseTuple(args, "O|i", &callback, &wait))
+    {
+        return NULL;
+    }
 
     test_c_thread.start_event = PyThread_allocate_lock();
     test_c_thread.exit_event = PyThread_allocate_lock();
@@ -4271,6 +4278,10 @@ call_in_temporary_c_thread(PyObject *self, PyObject *callback)
     PyThread_acquire_lock(test_c_thread.start_event, 1);
     PyThread_release_lock(test_c_thread.start_event);
 
+    if (!wait) {
+        Py_RETURN_NONE;
+    }
+
     Py_BEGIN_ALLOW_THREADS
         PyThread_acquire_lock(test_c_thread.exit_event, 1);
         PyThread_release_lock(test_c_thread.exit_event);
@@ -4281,13 +4292,32 @@ call_in_temporary_c_thread(PyObject *self, PyObject *callback)
 
 exit:
     Py_CLEAR(test_c_thread.callback);
-    if (test_c_thread.start_event)
+    if (test_c_thread.start_event) {
         PyThread_free_lock(test_c_thread.start_event);
-    if (test_c_thread.exit_event)
+        test_c_thread.start_event = NULL;
+    }
+    if (test_c_thread.exit_event) {
         PyThread_free_lock(test_c_thread.exit_event);
+        test_c_thread.exit_event = NULL;
+    }
     return res;
 }
 
+static PyObject *
+join_temporary_c_thread(PyObject *self, PyObject *Py_UNUSED(ignored))
+{
+    Py_BEGIN_ALLOW_THREADS
+        PyThread_acquire_lock(test_c_thread.exit_event, 1);
+        PyThread_release_lock(test_c_thread.exit_event);
+    Py_END_ALLOW_THREADS
+    Py_CLEAR(test_c_thread.callback);
+    PyThread_free_lock(test_c_thread.start_event);
+    test_c_thread.start_event = NULL;
+    PyThread_free_lock(test_c_thread.exit_event);
+    test_c_thread.exit_event = NULL;
+    Py_RETURN_NONE;
+}
+
 /* marshal */
 
 static PyObject*
@@ -5532,8 +5562,9 @@ static PyMethodDef TestMethods[] = {
     {"docstring_with_signature_with_defaults",
         (PyCFunction)test_with_docstring, METH_NOARGS,
         docstring_with_signature_with_defaults},
-    {"call_in_temporary_c_thread", call_in_temporary_c_thread, METH_O,
+    {"call_in_temporary_c_thread", call_in_temporary_c_thread, METH_VARARGS,
      PyDoc_STR("set_error_class(error_class) -> None")},
+    {"join_temporary_c_thread", join_temporary_c_thread, METH_NOARGS},
     {"pymarshal_write_long_to_file",
         pymarshal_write_long_to_file, METH_VARARGS},
     {"pymarshal_write_object_to_file",
diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c
@@ -801,6 +801,11 @@ local_traverse(localobject *self, visitproc visit, void *arg)
     return 0;
 }
 
+#define HEAD_LOCK(runtime) \
+    PyThread_acquire_lock((runtime)->interpreters.mutex, WAIT_LOCK)
+#define HEAD_UNLOCK(runtime) \
+    PyThread_release_lock((runtime)->interpreters.mutex)
+
 static int
 local_clear(localobject *self)
 {
@@ -810,17 +815,26 @@ local_clear(localobject *self)
     Py_CLEAR(self->dummies);
     Py_CLEAR(self->wr_callback);
     /* Remove all strong references to dummies from the thread states */
-    if (self->key
-        && (tstate = PyThreadState_Get())
-        && tstate->interp) {
-        for(tstate = PyInterpreterState_ThreadHead(tstate->interp);
-            tstate;
-            tstate = PyThreadState_Next(tstate))
-            if (tstate->dict && PyDict_GetItem(tstate->dict, self->key)) {
-                if (PyDict_DelItem(tstate->dict, self->key)) {
+    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) {
+                PyObject *v = _PyDict_Pop(tstate->dict, self->key, Py_None);
+                if (v != NULL) {
+                    Py_DECREF(v);
+                }
+                else {
                     PyErr_Clear();
                 }
             }
+            HEAD_LOCK(runtime);
+            tstate = PyThreadState_Next(tstate);
+            HEAD_UNLOCK(runtime);
+        }
     }
     return 0;
 }

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	+Fix race while iterating over thread states in clearing :class:`threading.local`. Patch by Kumar Aditya.