gh-154937: Fix _thread._shutdown() racing with _thread.start_joinable_thread on ThreadHandle.ident

[LindaSummer]

Issue

Close #154937

Proposed Changes

The ThreadHandle.ident should be guarded by ThreadHandle.mutex as the below comment.

cpython/Modules/_threadmodule.c

Lines 132 to 136 in 083e038

	// The `ident`, `os_handle`, `has_os_handle`, and `state` fields are
	// protected by `mutex`.
	PyThread_ident_t ident;
	PyThread_handle_t os_handle;
	int has_os_handle;

Root Cause

The thread.Thread will invoke _start_joinable_thread with the same flag as the invoke thread. The main thread is a non-daemon thread.

cpython/Lib/threading.py

Lines 1111 to 1147 in 083e038

	def start(self):
	"""Start the thread's activity.
	It must be called at most once per thread object. It arranges for the
	object's run() method to be invoked in a separate thread of control.
	This method will raise a RuntimeError if called more than once on the
	same thread object.
	"""
	if not self._initialized:
	raise RuntimeError("thread.__init__() not called")
	if self._started.is_set():
	raise RuntimeError("threads can only be started once")
	with _active_limbo_lock:
	_limbo[self] = self
	if self._context is None:
	# No context provided
	if _sys.flags.thread_inherit_context:
	# start with a copy of the context of the caller
	self._context = _contextvars.copy_context()
	else:
	# start with an empty context
	self._context = _contextvars.Context()
	try:
	# Start joinable thread
	_start_joinable_thread(self._bootstrap, handle=self._os_thread_handle,
	daemon=self.daemon)
	except Exception:
	with _active_limbo_lock:
	del _limbo[self]
	raise
	self._started.wait() # Will set ident and native_id

cpython/Lib/threading.py

Lines 1527 to 1537 in 083e038

	class _MainThread(Thread):
	def __init__(self):
	Thread.__init__(self, name="MainThread", daemon=False)
	self._started.set()
	self._ident = _get_main_thread_ident()
	self._os_thread_handle = _make_thread_handle(self._ident)
	if _HAVE_THREAD_NATIVE_ID:
	self._set_native_id()
	with _active_limbo_lock:
	_active[self._ident] = self

In _start_joinable_thread=>do_start_new_thread, the non-daemon thread will add to the shutdown_handles.

cpython/Modules/_threadmodule.c

Lines 1912 to 1917 in 083e038

	if (!daemon) {
	// Add the handle before starting the thread to avoid adding a handle
	// to a thread that has already finished (i.e. if the thread finishes
	// before the call to `ThreadHandle_start()` below returns).
	add_to_shutdown_handles(state, handle);
	}

After being added to shutdown_handlers, do_start_new_thread will invoke ThreadHandle_start, where the racing happened.

cpython/Modules/_threadmodule.c

Lines 483 to 490 in 083e038

	// Mark the handle running
	PyMutex_Lock(&self->mutex);
	assert(self->state == THREAD_HANDLE_STARTING);
	self->ident = ident;
	self->has_os_handle = 1;
	self->os_handle = os_handle;
	self->state = THREAD_HANDLE_RUNNING;
	PyMutex_Unlock(&self->mutex);

At the same time, _shutdown will read the ident for shutting down without a lock with the ThreadHandle.mutex.

cpython/Modules/_threadmodule.c

Lines 2403 to 2422 in 083e038

	thread_shutdown(PyObject *self, PyObject *args)
	{
	PyThread_ident_t ident = PyThread_get_thread_ident_ex();
	thread_module_state *state = get_thread_state(self);
	for (;;) {
	ThreadHandle *handle = NULL;
	// Find a thread that's not yet finished.
	HEAD_LOCK(&_PyRuntime);
	struct llist_node *node;
	llist_for_each_safe(node, &state->shutdown_handles) {
	ThreadHandle *cur = llist_data(node, ThreadHandle, shutdown_node);
	if (cur->ident != ident) {
	ThreadHandle_incref(cur);
	handle = cur;
	break;
	}
	}
	HEAD_UNLOCK(&_PyRuntime);

Comment

Referring to the above root cause analytics, I find the original test case may not be precise.

The original case in #154937 uses _thread.start_new_thread alongside threading.Thread.
The race was caused by threading.Thread not _thread.start_new_thread.

def chain2_thread():
    for _ in range(12000):
        try:
            _thread.start_new_thread(lambda: None, ())
        except Exception:
            pass
        time.sleep(0.0001)

...

threads += [Thread(target=chain2_thread) for _ in range(N_C2)]

So, I changed the original case to below with daemon=True, and the racing doesn't happen.

import _thread
import time
from threading import Thread

def chain1_thread():
    for _ in range(12000):
        try:
            _thread._shutdown()
        except Exception:
            pass

def chain2_thread():
    for _ in range(12000):
        try:
            _thread.start_new_thread(lambda: None, ())
        except Exception:
            pass
        time.sleep(0.0001)

N_C1 = 1
N_C2 = 8
threads  = [Thread(target=chain1_thread, daemon=True) for _ in range(N_C1)]
threads += [Thread(target=chain2_thread, daemon=True) for _ in range(N_C2)]
for t in threads: t.start()
for t in threads: t.join()

I created another case that aligns with the PR's unit test, and the problem is reproduced again.

import _thread
from threading import Barrier

ITERATIONS = 1_000
phase = Barrier(2)


def shutdown_worker():
    phase.wait()
    for _ in range(ITERATIONS):
        try:
            _thread._shutdown()
        except RuntimeError:
            pass

def startup_worker():
    phase.wait()
    for _ in range(ITERATIONS):
        handle = _thread.start_joinable_thread(
            lambda: None,
            daemon=False,
        )
        handle.join()

startup_worker = _thread.start_joinable_thread(
    startup_worker,
    daemon=True,
)

shutdown_worker = _thread.start_joinable_thread(
    shutdown_worker,
    daemon=True,
)
shutdown_worker.join()
startup_worker.join()

Here is the TSAN report.

WARNING: ThreadSanitizer: data race (pid=2708706)
  Read of size 8 at 0x7fca36010070 by thread T2:
    #0 thread_shutdown /home/someuser/projects/cpython/cpython-upstream-main/./Modules/_threadmodule.c:2416:22 (python3.16+0x653ba9)
    #1 cfunction_vectorcall_NOARGS /home/someuser/projects/cpython/cpython-upstream-main/Objects/methodobject.c:508:24 (python3.16+0x2ddf41)
    #2 _PyObject_VectorcallTstate /home/someuser/projects/cpython/cpython-upstream-main/./Include/internal/pycore_call.h:144:11 (python3.16+0x2240fc)
    #3 PyObject_Vectorcall /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c:327:12 (python3.16+0x2240fc)
    #4 _Py_VectorCall_StackRefSteal /home/someuser/projects/cpython/cpython-upstream-main/Python/ceval.c:726:11 (python3.16+0x457e76)
    #5 _PyEval_EvalFrameDefault /home/someuser/projects/cpython/cpython-upstream-main/Python/generated_cases.c.h:4559:35 (python3.16+0x465a0c)
    #6 _PyEval_EvalFrame /home/someuser/projects/cpython/cpython-upstream-main/./Include/internal/pycore_ceval.h:122:16 (python3.16+0x457987)
    #7 _PyEval_Vector /home/someuser/projects/cpython/cpython-upstream-main/Python/ceval.c:2172:12 (python3.16+0x457987)
    #8 _PyFunction_Vectorcall /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c (python3.16+0x2247c7)
    #9 _PyVectorcall_Call /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c:273:16 (python3.16+0x224416)
    #10 _PyObject_Call /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c:348:16 (python3.16+0x224416)
    #11 PyObject_Call /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c:373:12 (python3.16+0x22448b)
    #12 thread_run /home/someuser/projects/cpython/cpython-upstream-main/./Modules/_threadmodule.c:388:21 (python3.16+0x654aec)
    #13 pythread_wrapper /home/someuser/projects/cpython/cpython-upstream-main/Python/thread_pthread.h:234:5 (python3.16+0x574def)

  Previous write of size 8 at 0x7fca36010070 by thread T1:
    #0 ThreadHandle_start /home/someuser/projects/cpython/cpython-upstream-main/./Modules/_threadmodule.c:486:17 (python3.16+0x654966)
    #1 do_start_new_thread /home/someuser/projects/cpython/cpython-upstream-main/./Modules/_threadmodule.c:1919:9 (python3.16+0x6542ef)
    #2 thread_PyThread_start_joinable_thread /home/someuser/projects/cpython/cpython-upstream-main/./Modules/_threadmodule.c:2042:14 (python3.16+0x653268)
    #3 cfunction_call /home/someuser/projects/cpython/cpython-upstream-main/Objects/methodobject.c:564:18 (python3.16+0x2de9c5)
    #4 _PyObject_MakeTpCall /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c:242:18 (python3.16+0x2232b3)
    #5 _PyObject_VectorcallTstate /home/someuser/projects/cpython/cpython-upstream-main/./Include/internal/pycore_call.h:142:16 (python3.16+0x2241c4)
    #6 PyObject_Vectorcall /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c:327:12 (python3.16+0x2241c4)
    #7 _Py_VectorCallInstrumentation_StackRefSteal /home/someuser/projects/cpython/cpython-upstream-main/Python/ceval.c:768:11 (python3.16+0x4586f4)
    #8 _PyEval_EvalFrameDefault /home/someuser/projects/cpython/cpython-upstream-main/Python/generated_cases.c.h:3474:35 (python3.16+0x463029)
    #9 _PyEval_EvalFrame /home/someuser/projects/cpython/cpython-upstream-main/./Include/internal/pycore_ceval.h:122:16 (python3.16+0x457987)
    #10 _PyEval_Vector /home/someuser/projects/cpython/cpython-upstream-main/Python/ceval.c:2172:12 (python3.16+0x457987)
    #11 _PyFunction_Vectorcall /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c (python3.16+0x2247c7)
    #12 _PyVectorcall_Call /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c:273:16 (python3.16+0x224416)
    #13 _PyObject_Call /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c:348:16 (python3.16+0x224416)
    #14 PyObject_Call /home/someuser/projects/cpython/cpython-upstream-main/Objects/call.c:373:12 (python3.16+0x22448b)
    #15 thread_run /home/someuser/projects/cpython/cpython-upstream-main/./Modules/_threadmodule.c:388:21 (python3.16+0x654aec)
    #16 pythread_wrapper /home/someuser/projects/cpython/cpython-upstream-main/Python/thread_pthread.h:234:5 (python3.16+0x574def)

• Issue: Data race on ThreadHandle.ident between thread_shutdown and ThreadHandle_start under free-threading #154937

[LindaSummer]

Hi @aisk ,

Could you help take a look at this patch at your convenience?

Thank you, and have a great day!

[LindaSummer]

Hi @aisk ,

Thanks very much for your review and suggestions!

I've updated the patch based on the feedback.
When you have a chance, could you please take another look?

Have a wonderful day!

[kumaraditya303]

The data race is trivial, I suggest to not add such heavy test for it, you can remove the test.

[LindaSummer]

The data race is trivial, I suggest to not add such heavy test for it, you can remove the test.

Hi @kumaraditya303 ,

Thanks very much for your review!

I have removed the heavy 4*1000 threads unit test.

Please help take a look at your convenience.

Have a great day!

[aisk]

(Checked that this PR can be backported to 3.13-3.15 without conflict via opencode + glm5.2)

[miss-islington-app]

Thanks @LindaSummer for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15.
🐍🍒⛏🤖

[bedevere-app]

GH-155123 is a backport of this pull request to the 3.15 branch.

[bedevere-app]

GH-155124 is a backport of this pull request to the 3.14 branch.

[bedevere-app]

GH-155125 is a backport of this pull request to the 3.13 branch.