From 683e9fe30ecd024f5508b2a33316752870100a96 Mon Sep 17 00:00:00 2001 From: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Date: Wed, 11 Jan 2023 16:03:31 +0530 Subject: [PATCH] [3.10] GH-100892: Fix race in clearing `threading.local` (GH-100922). (cherry picked from commit 762745a124cbc297cf2fe6f3ec9ca1840bb2e873) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> --- Lib/test/test_threading_local.py | 13 ++++++ ...-01-10-14-11-17.gh-issue-100892.qfBVYI.rst | 1 + Modules/_testcapimodule.c | 41 ++++++++++++++++--- Modules/_threadmodule.c | 30 +++++++++----- 4 files changed, 70 insertions(+), 15 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-01-10-14-11-17.gh-issue-100892.qfBVYI.rst diff --git a/Lib/test/test_threading_local.py b/Lib/test/test_threading_local.py index 13facb513367a4..f1ef30e8f1a0ba 100644 --- a/Lib/test/test_threading_local.py +++ b/Lib/test/test_threading_local.py @@ -3,6 +3,7 @@ from doctest import DocTestSuite from test import support from test.support import threading_helper +from test.support.import_helper import import_module import weakref import gc @@ -194,6 +195,18 @@ class X: self.assertIsNone(wr()) + def test_threading_local_clear_race(self): + # See https://github.com/python/cpython/issues/100892 + + _testcapi = import_module('_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 new file mode 100644 index 00000000000000..f2576becc2fcfc --- /dev/null +++ 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 index c0c86722eb3c34..bafd0c4e697a83 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -4368,12 +4368,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(); @@ -4400,6 +4407,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); @@ -4410,13 +4421,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* @@ -5878,8 +5908,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 index 813d7ec232c35d..8ac75b91097a73 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -842,6 +842,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) { @@ -852,18 +857,23 @@ local_clear(localobject *self) /* 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); - for(; tstate; tstate = PyThreadState_Next(tstate)) { - if (tstate->dict == NULL) { - continue; - } - PyObject *v = _PyDict_Pop(tstate->dict, self->key, Py_None); - if (v != NULL) { - Py_DECREF(v); - } - else { - PyErr_Clear(); + 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;