8000 gh-114271: Fix race in `Thread.join()` by mpage · Pull Request #114839 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

gh-114271: Fix race in Thread.join() #114839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 34 commits into from
Mar 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
fb13cf6
Fix race in `Thread.join()`
mpage Jan 26, 2024
b093e5d
📜🤖 Added by blurb_it.
blurb-it[bot] Feb 1, 2024
1039208
Fix NEWS entry
mpage Feb 1, 2024
196081d
Work around c-analyzer limitation
mpage Feb 5, 2024
19d7af1
Merge branch 'main' into gh-114271-remove-tstate_lock
mpage Feb 5, 2024
c86d349
Merge branch 'main' into gh-114271-remove-tstate_lock
mpage Feb 7, 2024
40e4b36
Merge branch 'main' into gh-114271-remove-tstate_lock
mpage Feb 14, 2024
4ed1083
Merge branch 'main' into gh-114271-remove-tstate_lock
mpage Feb 14, 2024
34b6065
Merge branch 'main' into gh-114271-remove-tstate_lock
mpage Mar 1, 2024
1c82786
Fix two compilation errors post merge
mpage Mar 1, 2024
0e86cf9
Use ThreadHandle as the single abstraction for thread joining
mpage Mar 2, 2024
76bde03
Isolate all logic in _threadmodule
mpage Mar 4, 2024
02123b8
Fix flag
mpage Mar 5, 2024
24c1d47
Note that the once flag serializes both join and set_done
mpage Mar 6, 2024
1a1bfde
Fix unused variable warning
mpage Mar 6, 2024
b3c2c45
Rename ThreadHandleObject to PyThreadHandleObject
mpage Mar 6, 2024
7b9d007
Be consistent with documentation of true values
mpage Mar 6, 2024
5d0dc7a
Threads started using `start_joinable_thread` should be daemon thread…
mpage Mar 6, 2024
11bb826
Remove any remaining handles once the module is cleared
mpage Mar 7, 2024
06f6787
Always have a _ThreadHandle in Thread
mpage Mar 8, 2024
d56f892
Check main thread handle during shutdown
mpage Mar 8, 2024
57e106d
Merge branch 'main' into gh-114271-remove-tstate_lock
mpage Mar 8, 2024
2218c0a
Remove vestigial _PyEventRc declarations
mpage Mar 8, 2024
f9d3290
Remove duplicate declaration
mpage Mar 8, 2024
a7095e4
Revert order change in `_DummyThread.is_alive`
mpage Mar 8, 2024
ccd1c2e
Merge branch 'main' into gh-114271-remove-tstate_lock
mpage Mar 11, 2024
180300c
Add handles to shutdown list before starting the thread
mpage Mar 11, 2024
c486503
Move some code around to remove need for forward decls
mpage Mar 12, 2024
3121623
Simplify start failure path
mpage Mar 12, 2024
ee96259
Merge branch 'main' into gh-114271-remove-tstate_lock
mpage Mar 15, 2024
dc57ed2
Use infinitive in docstring for _shutdown
mpage Mar 15, 2024
48b86ae
Update comment for _make_thread_handle
mpage Mar 15, 2024
9a8ea9b
Merge branch 'main' into gh-114271-remove-tstate_lock
pitrou Mar 16, 2024
339b2e6
Merge branch 'main' into gh-114271-remove-tstate_lock
pitrou Mar 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Use ThreadHandle as the single abstraction for thread joining
  • Loading branch information
mpage committed Mar 2, 2024
commit 0e86cf97670d54391cf9f3a2fa444009e8b0a97c
12 changes: 8 additions & 4 deletions Include/cpython/pystate.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,16 @@ struct _ts {
*/
uintptr_t critical_section;

/* Boolean storing whether or not this is a daemon thread. All non-daemon
* threads are joined prior to interpreter exit.
*/
/* Boolean storing whether or not this is a daemon thread. */
int is_daemon;

/* Set when the thread has finished execution and is about to be freed. */
/* Set when the tstate has been cleared and unlinked from the list of
* active tstates.
*
* This is used by _PyInterpreterState_WaitForThreads to wait for all
* non-daemon threads to finish. It cannot be a PyObject because its
* lifetime exceeds the tstate to which it is bound.
*/
struct _PyEventRc *done_event;

int coroutine_origin_tracking_depth;
Expand Down
3 changes: 0 additions & 3 deletions Include/internal/pycore_pystate.h
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,6 @@ static inline PyInterpreterState* _PyInterpreterState_GET(void) {
extern PyThreadState * _PyThreadState_New(
PyInterpreterState *interp,
int whence);
extern PyThreadState *
_PyThreadState_NewWithEvent(PyInterpreterState *interp, int whence,
_PyEventRc *done_event);
extern void _PyThreadState_Bind(PyThreadState *tstate);
extern void _PyThreadState_DeleteExcept(PyThreadState *tstate);
extern void _PyThreadState_ClearMimallocHeaps(PyThreadState *tstate);
Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_audit.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,9 @@ def test_threading(self):
print(*events, sep='\n')
actual = [(ev[0], ev[2]) for ev in events]
expected = [
("_thread.start_new_thread", "(<test_func>, (), None, None, 0)"),
("_thread.start_new_thread", "(<test_func>, (), None)"),
("test.test_func", "()"),
("_thread.start_joinable_thread", "(<test_func>, None, 0)"),
("_thread.start_joinable_thread", "(<test_func>, 0)"),
("test.test_func", "()"),
]

Expand Down
15 changes: 15 additions & 0 deletions Lib/test/test_thread.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,21 @@ def joiner():
with self.assertRaisesRegex(RuntimeError, "Cannot join current thread"):
raise error

def test_join_with_timeout(self):
lock = thread.allocate_lock()
lock.acquire()

def thr():
lock.acquire()

with threading_helper.wait_threads_exit():
handle = thread.start_joinable_thread(thr)
handle.join(0.1)
self.assertFalse(handle.is_done())
lock.release()
handle.join()
self.assertTrue(handle.is_done())


class Barrier:
def __init__(self, num_threads):
Expand Down
24 changes: 0 additions & 24 deletions Lib/test/test_threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -901,30 +901,6 @@ def f():
rc, out, err = assert_python_ok("-c", code)
self.assertEqual(err, b"")

@cpython_only
def test_done_event(self):
# Test an implementation detail of Thread objects.
started = _thread.allocate_lock()
finish = _thread.allocate_lock()
started.acquire()
finish.acquire()
def f():
started.release()
finish.acquire()
time.sleep(0.01)
# The done event is not set if the thread hasn't started
t = threading.Thread(target=f)
self.assertFalse(t._done_event.is_set())
t.start()
started.acquire()
self.assertTrue(t.is_alive())
# The done event is not set while the thread is alive
self.assertFalse(t._done_event.wait(0), False)
finish.release()
# When the thread ends, the done event is set.
self.assertTrue(t._done_event.wait(support.SHORT_TIMEOUT), False)
t.join()

def test_repr_stopped(self):
# Verify that "stopped" shows up in repr(Thread) appropriately.
started = _thread.allocate_lock()
Expand Down
52 changes: 18 additions & 34 deletions Lib/threading.py
< 5C47 td class="blob-code blob-code-deletion js-file-line"> # It should have been set already by
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,8 @@
_daemon_threads_allowed = _thread.daemon_threads_allowed
_allocate_lock = _thread.allocate_lock
_LockType = _thread.LockType
_Event = _thread.Event
_get_done_event = _thread._get_done_event
_thread_shutdown = _thread._shutdown
_get_thread_handle = _thread._get_thread_handle
get_ident = _thread.get_ident
_is_main_interpreter = _thread._is_main_interpreter
try:
Expand Down Expand Up @@ -915,7 +914,6 @@ class is implemented.
self._native_id = None
self._handle = None
self._started = Event()
self._done_event = _Event()
self._initialized = True
# Copy of sys.stderr used by self._invoke_excepthook()
self._stderr = _sys.stderr
Expand All @@ -932,17 +930,16 @@ def _after_fork(self, new_ident=None):
if self._handle is not None:
assert self._handle.ident == new_ident
else:
# This thread isn't alive after fork: it doesn't have a tstate
# anymore.
self._done_event.set()
self._handle = None
# Otherwise, the thread is dead, Jim. If we had a handle
# _PyThread_AfterFork() already marked it done.
pass

def __repr__(self):
assert self._initialized, "Thread.__init__() was not called"
status = "initial"
if self._started.is_set():
status = "started"
if self._done_event.is_set():
if self._handle and self._handle.is_done():
status = "stopped"
if self._daemonic:
status += " daemon"
Expand Down Expand Up @@ -970,7 +967,7 @@ def start(self):
_limbo[self] = self
try:
# Start joinable thread
self._handle = _start_joinable_thread(self._bootstrap, done_event=self._done_event, daemon=self.daemon)
self._handle = _start_joinable_thread(self._bootstrap, daemon=self.daemon)
except Exception:
with _active_limbo_lock:
del _limbo[self]
Expand Down Expand Up @@ -1087,15 +1084,8 @@ def join(self, timeout=None):
# historically .join(timeout=x) for x<0 has acted as if timeout=0
if timeout is not None:
timeout = max(timeout, 0)
self._done_event.wait(timeout)

if self._done_event.is_set():
self._join_os_thread()

def _join_os_thread(self):
# self._handle may be cleared post-fork
if self._handle is not None:
self._handle.join()
self._handle.join(timeout)

@property
def name(self):
Expand Down Expand Up @@ -1146,7 +1136,7 @@ def is_alive(self):

"""
assert self._initialized, "Thread.__init__() not called"
return self._started.is_set() and not self._done_event.is_set()
return self._started.is_set() and not self._handle.is_done()

@property
def daemon(self):
Expand Down Expand Up @@ -1355,18 +1345,14 @@ class _MainThread(Thread):

def __init__(self):
Thread.__init__(self, name="MainThread", daemon=False)
self._done_event = _get_done_event()
self._started.set()
self._set_ident()
self._handle = _get_thread_handle()
if _HAVE_THREAD_NATIVE_ID:
self._set_native_id()
with _active_limbo_lock:
_active[self._ident] = self

def _join_os_thread(self):
# No ThreadHandle for main thread
pass


# Helper thread-local instance to detect when a _DummyThread
# is collected. Not a part of the public API.
Expand Down Expand Up @@ -1407,17 +1393,17 @@ class _DummyThread(Thread):
def __init__(self):
Thread.__init__(self, name=_newname("Dummy-%d"),
daemon=_daemon_threads_allowed())
self._done_event = _get_done_event()
self._started.set()
self._set_ident()
self._handle = _get_thread_handle()
if _HAVE_THREAD_NATIVE_ID:
self._set_native_id()
with _active_limbo_lock:
_active[self._ident] = self
_DeleteDummyThreadOnDel(self)

def is_alive(self):
if self._started.is_set() and not self._done_event.is_set():
if self._started.is_set() and not self._handle.is_done():
return True
raise RuntimeError("thread is not alive")

Expand Down Expand Up @@ -1528,11 +1514,11 @@ def _shutdown():
Wait until the Python thread state of all non-daemon threads get deleted.
"""
global _SHUTTING_DOWN
# Obscure: other threads may be waiting to join _main_thread. That's
# dubious, but some code does it. We can't wait for C code to set
# the main thread's done_event - that won't happen until the interpreter
# is nearly dead. So we set it here.
if _main_thread._done_event.is_set() and _is_main_interpreter() and _SHUTTING_DOWN:
# Obscure: other threads may be waiting to join _main_thread. That's
# dubious, but some code does it. We can't wait for it to be marked as done
# normally - that won't happen until the interpreter is nearly dead. So
# mark it done here.
if _is_main_interpreter() and _SHUTTING_DOWN:
# _shutdown() was already called
return

Expand All @@ -1543,11 +1529,9 @@ def _shutdown():
for atexit_call in reversed(_threading_atexits):
atexit_call()

# Main thread
if _main_thread.ident == get_ident():
# _PyInterpreterState_SetNotRunningMain(), but there may be embedders
# that aren't calling that yet.
_main_thread._done_event.set()
_main_thread._handle._set_done()
else:
# bpo-1596321: _shutdown() must be called in the main thread.
# If the threading module was not imported by the main thread,
Expand Down
Loading
0