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

mpage · 2024-02-01T03:04:54Z

This PR makes thread joining thread-safe in free-threaded builds and fixes gh-116372, which affects both default and free-threaded builds.

The combination of _tstate_lock and _ThreadHandle is replaced by _ThreadHandle and the supporting infrastructure for _tstate_lock is removed. Joining a thread now joins its _ThreadHandle. The _ThreadHandles for all threads that we want to join on shutdown (non-daemon threads that were created by the threading module) are tracked in a linked-list attached to the _thread module. At shutdown we iterate over this list and join the handles.

There is one wrinkle with this approach: the _MainThread instance. The _MainThread instance is joinable. Previously, it represented whatever thread imported the threading module. Typically this is the main thread of the main interpreter, but in some rare circumstances (embedding scenarios?) it might not be. The previous approach allowed the MainThread instance to be joined even if it wasn't the main thread of the main interpreter, but this isn't possible with the current approach. To work around this we change the _MainThread to always represent the main thread of the runtime, also fixing gh-83223.

Issue: Make _threadmodule.c thread-safe in --disable-gil builds #114271

There is a race between when `Thread._tstate_lock` is released[^1] in `Thread._wait_for_tstate_lock()` and when `Thread._stop()` asserts[^2] that it is unlocked. Consider the following execution involving threads A, B, and C: 1. A starts. 2. B joins A, blocking on its `_tstate_lock`. 3. C joins A, blocking on its `_tstate_lock`. 4. A finishes and releases its `_tstate_lock`. 5. B acquires A's `_tstate_lock` in `_wait_for_tstate_lock()`, releases it, but is swapped out before calling `_stop()`. 6. C is scheduled, acquires A's `_tstate_lock` in `_wait_for_tstate_lock()` but is swapped out before releasing it. 7. B is scheduled, calls `_stop()`, which asserts that A's `_tstate_lock` is not held. However, C holds it, so the assertion fails. The race can be reproduced[^3] by inserting sleeps at the appropriate points in the threading code. To do so, run the `repro_join_race.py` from the linked repo. There are two main parts to this PR: 1. `_tstate_lock` is replaced with an event that is attached to `PyThreadState`. The event is set by the runtime prior to the thread being cleared (in the same place that `_tstate_lock` was released). `Thread.join()` blocks waiting for the event to be set. 2. `_PyInterpreterState_WaitForThreads()` provides the ability to wait for all non-daemon threads to exit. To do so, an `is_daemon` predicate was added to `PyThreadState`. This field is set each time a thread is created. `threading._shutdown()` now calls into `_PyInterpreterState_WaitForThreads()` instead of waiting on `_tstate_lock`s. [^1]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1201 [^2]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1115 [^3]: 8194653

mpage · 2024-02-01T17:26:41Z

Just a heads up - please don't look at this until it's marked as ready for review. There are a couple of failing CI jobs that need to be addressed.

When we have `typedef struct _PyEventRc _PyEventRc` in `pstate.h` the C analyzer can't decide between that and the definition in `pycore_lock.h`.

mpage · 2024-02-06T17:20:57Z

@colesbury - This is good to go. Would you please have a look?

mpage · 2024-02-06T19:53:08Z

Putting this back in draft. Going to see if I can move the join logic into ThreadHandle.

mpage · 2024-03-06T17:49:48Z

@colesbury - Could you take a look at this when you get a chance, please?

colesbury

Overall, the approach looks nice!

I'm still looking this over, but I have some questions about thread._handle. Currently, thread._handle starts out as None and is then set to a ThreadHandle after the thread is started. This None -> ThreadHandle transition seems like it adds complexity and I think there may be a few bugs related to it (see comments below).

Would it be possible to always have a non-None self._handle by pushing a bit more state into ThreadHandle?

Lib/threading.py

Python/pystate.c

ericsnowcurrently · 2024-03-14T17:51:17Z

Yeah, you've captured it well. FYI, I was looking at the _MainThread stuff separately already. 😄

ericsnowcurrently · 2024-03-14T17:52:01Z

I'd be glad to follow-through on the changes I posted in gh-116776, if that would help you.

mpage · 2024-03-14T21:01:17Z

I'd be glad to follow-through on the changes I posted in #116776, if that would help you.

@ericsnowcurrently - Thanks! Is that something you think you can get done quickly (i.e. in the next few days)? If so, I'd be happy to stack on top of those changes once you're done.

ericsnowcurrently · 2024-03-14T22:26:34Z

Yeah, I'll aim to wrap it up tomorrow.

pitrou

+1. I really like the simplification here!

Modules/_threadmodule.c

ericsnowcurrently · 2024-03-15T16:10:33Z

I'll defer to @pitrou's assessment on this. I'd rather this land sooner than later and don't want my pace to hold things up. Again, thanks for working on this.

(I'll follow up separately on any cleanup, about the "main" thread, and about our approach to managing thread lifetimes. I actually appreciate not feeling so rushed to sort those out. 😄)

ericsnowcurrently · 2024-03-15T16:12:10Z

FWIW, this change feels a bit closer to what I had in mind last year: #110848 (comment).

mpage · 2024-03-15T16:46:15Z

The CIFuzz (memory) failure is unrelated to this PR.

pitrou · 2024-03-15T16:50:41Z

Do we have a Thread Sanitizer CI build somewhere? In my experience it's quite good at finding synchronization bugs.
@gpshead

Edit: oh, I see this is being proposed as #116872. Thank you @corona10 !

pitrou · 2024-03-15T17:16:02Z

@gpshead Would you like to take a look at this?

pitrou · 2024-03-16T10:19:07Z

Ok, TSAN tests passed after merging from main (though there are too few of them currently).

pitrou · 2024-03-16T11:56:19Z

Updated again to get test_threading in the set of TSAN tests.

There is a race between when `Thread._tstate_lock` is released[^1] in `Thread._wait_for_tstate_lock()` and when `Thread._stop()` asserts[^2] that it is unlocked. Consider the following execution involving threads A, B, and C: 1. A starts. 2. B joins A, blocking on its `_tstate_lock`. 3. C joins A, blocking on its `_tstate_lock`. 4. A finishes and releases its `_tstate_lock`. 5. B acquires A's `_tstate_lock` in `_wait_for_tstate_lock()`, releases it, but is swapped out before calling `_stop()`. 6. C is scheduled, acquires A's `_tstate_lock` in `_wait_for_tstate_lock()` but is swapped out before releasing it. 7. B is scheduled, calls `_stop()`, which asserts that A's `_tstate_lock` is not held. However, C holds it, so the assertion fails. The race can be reproduced[^3] by inserting sleeps at the appropriate points in the threading code. To do so, run the `repro_join_race.py` from the linked repo. There are two main parts to this PR: 1. `_tstate_lock` is replaced with an event that is attached to `PyThreadState`. The event is set by the runtime prior to the thread being cleared (in the same place that `_tstate_lock` was released). `Thread.join()` blocks waiting for the event to be set. 2. `_PyInterpreterState_WaitForThreads()` provides the ability to wait for all non-daemon threads to exit. To do so, an `is_daemon` predicate was added to `PyThreadState`. This field is set each time a thread is created. `threading._shutdown()` now calls into `_PyInterpreterState_WaitForThreads()` instead of waiting on `_tstate_lock`s. [^1]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1201 [^2]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1115 [^3]: mpage@8194653 --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <antoine@python.org>

…hon 3.13 `Thread._is_stopped` was removed in python/cpython#114839 in Python 3.13 (cherry picked from commit d568bc288aed00268ffeef137b9b901f480964ef) IJ-MR-137093 GitOrigin-RevId: aa8b27b09db85f69b44d6fbbd333f8ec32489d33

bedevere-app bot mentioned this pull request Feb 1, 2024

Make _threadmodule.c thread-safe in --disable-gil builds #114271

Closed

blurb-it bot and others added 2 commits February 1, 2024 03:09

📜🤖 Added by blurb_it.

b093e5d

Fix NEWS entry

1039208

Eclips4 requested a review from colesbury February 1, 2024 05:55

Eclips4 added the topic-free-threading label Feb 1, 2024

mpage mentioned this pull request Feb 3, 2024

Race between _PyParkingLot_Park and _PyParkingLot_UnparkAll when handling interrupts #114944

Closed

mpage added 2 commits February 5, 2024 13:26

Work around c-analyzer limitation

196081d

When we have `typedef struct _PyEventRc _PyEventRc` in `pstate.h` the C analyzer can't decide between that and the definition in `pycore_lock.h`.

Merge branch 'main' into pythongh-114271-remove-tstate_lock

19d7af1

mpage marked this pull request as ready for review February 5, 2024 22:49

mpage requested a review from ericsnowcurrently as a code owner February 5, 2024 22:49

bedevere-app bot added the awaiting review label Feb 5, 2024

mpage marked this pull request as draft February 6, 2024 19:52

bedevere-app bot removed the awaiting review label Feb 6, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

c86d349

colesbury mentioned this pull request Feb 9, 2024

gh-114271: Make _thread.ThreadHandle thread-safe in free-threaded builds #115190

Merged

mpage added 7 commits February 13, 2024 20:20

Merge branch 'main' into pythongh-114271-remove-tstate_lock

40e4b36

Merge branch 'main' into pythongh-114271-remove-tstate_lock

4ed1083

Merge branch 'main' into pythongh-114271-remove-tstate_lock

34b6065

Fix two compilation errors post merge

1c82786

Use ThreadHandle as the single abstraction for thread joining

0e86cf9

Isolate all logic in _threadmodule

76bde03

Fix flag

02123b8

mpage marked this pull request as ready for review March 5, 2024 20:30

bedevere-app bot added the awaiting review label Mar 5, 2024

colesbury reviewed Mar 6, 2024

View reviewed changes

Lib/threading.py Show resolved Hide resolved

Python/pystate.c Outdated Show resolved Hide resolved

pitrou approved these changes Mar 15, 2024

View reviewed changes

Modules/_threadmodule.c Show resolved Hide resolved

Modules/_threadmodule.c Outdated Show resolved Hide resolved

Modules/_threadmodule.c Outdated Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting changes labels Mar 15, 2024

mpage added 3 commits March 15, 2024 09:20

Merge branch 'main' into pythongh-114271-remove-tstate_lock

ee96259

Use infinitive in docstring for _shutdown

dc57ed2

Update comment for _make_thread_handle

48b86ae

Merge branch 'main' into pythongh-114271-remove-tstate_lock

9a8ea9b

Merge branch 'main' into pythongh-114271-remove-tstate_lock

339b2e6

pitrou merged commit 33da0e8 into python:main Mar 16, 2024
35 of 36 checks passed

bedevere-app bot removed the awaiting merge label Mar 16, 2024

pajod mentioned this pull request May 14, 2024

API calls getting stuck indefinitely possibiliy due to error "gevent.exceptions.LoopExit" benoitc/gunicorn#3117

Closed

aisk mentioned this pull request Jun 4, 2024

Use the runtime's main thread ID in the threading module. #83223

Open

rominf mentioned this pull request Jul 5, 2024

Test failure with Python 3.13 gevent/gevent#2037

Closed

hauntsaninja mentioned this pull request Sep 15, 2024

Daemonic threads not killed in some circumstances in python 3.13 #123940

Open

ericsnowcurrently mentioned this pull request Sep 17, 2024

gh-123940: Ensure force-terminated daemon threads can be joined #124150

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mpage commented Feb 1, 2024 •

edited

Loading

mpage commented Feb 1, 2024

mpage commented Feb 6, 2024

mpage commented Feb 6, 2024

mpage commented Mar 6, 2024

colesbury left a comment

ericsnowcurrently commented Mar 14, 2024

ericsnowcurrently commented Mar 14, 2024

mpage commented Mar 14, 2024

ericsnowcurrently commented Mar 14, 2024

pitrou left a comment

ericsnowcurrently commented Mar 15, 2024

ericsnowcurrently commented Mar 15, 2024

mpage commented Mar 15, 2024 •

edited

Loading

pitrou commented Mar 15, 2024 •

edited

Loading

pitrou commented Mar 15, 2024

pitrou commented Mar 16, 2024

pitrou commented Mar 16, 2024

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

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

Conversation

mpage commented Feb 1, 2024 • edited Loading

mpage commented Feb 1, 2024

mpage commented Feb 6, 2024

mpage commented Feb 6, 2024

mpage commented Mar 6, 2024

colesbury left a comment

Choose a reason for hiding this comment

ericsnowcurrently commented Mar 14, 2024

ericsnowcurrently commented Mar 14, 2024

mpage commented Mar 14, 2024

ericsnowcurrently commented Mar 14, 2024

pitrou left a comment

Choose a reason for hiding this comment

ericsnowcurrently commented Mar 15, 2024

ericsnowcurrently commented Mar 15, 2024

mpage commented Mar 15, 2024 • edited Loading

pitrou commented Mar 15, 2024 • edited Loading

pitrou commented Mar 15, 2024

pitrou commented Mar 16, 2024

pitrou commented Mar 16, 2024

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

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

mpage commented Feb 1, 2024 •

edited

Loading

mpage commented Mar 15, 2024 •

edited

Loading

pitrou commented Mar 15, 2024 •

edited

Loading