gh-114271: Make `_thread.ThreadHandle` thread-safe in free-threaded builds by mpage · Pull Request #115190 · python/cpython · GitHub

We protect the mutable state of ThreadHandle using a _PyOnceFlag. Concurrent operations (i.e. join) on ThreadHandle block until it is their turn to execute or an earlier operation succeeds. Once an operation has been applied successfully all future operations complete immediately.

The join() method is now idempotent. It may be called multiple times but the underlying OS thread will only be joined once. After join() succeeds, any future calls to join() will succeed immediately.

The detach() method has been removed; nothing was using it.

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

We protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`. Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` block until it is their turn to execute or an earlier operation succeeds. Once an operation has been applied successfully all future operations complete immediately. The `join()` method is now idempotent. It may be called multiple times but the underlying OS thread will only be joined once. After `join()` succeeds, any future calls to `join()` will succeed immediately. The `detach()` method is also idempotent. It may be called multiple times but the underlying OS thread will only be detached once. After `detach()` succeeds, any future calls to `detach()` will succeed immediately. If the handle is being joined, `detach()` blocks until the join completes.

@colesbury - Would you please have a look and add the "skip news" label to this? In order to limit the size of the changes I'd like to get this merged first and then stack the tstate_lock stuff on top.

@colesbury - This should be good to go. Would you please have another look?

Oh, the thread id reuse is tricky.

I'd like to avoid adding _PyEventRc if possible. The event doesn't seem like a great fit for ThreadHandleObject because we are never waiting on the event -- we're just using it like an atomic flag.

What about something like:

Make thread_is_exiting a plain variable that we use atomic operations to access, like int thread_is_exiting (because we don't have atomic ops on bool currently)
Pass the ThreadHandleObject to do_start_new_thread instead of having another object with a separate lifetime.

I'd like to avoid adding _PyEventRc if possible.

Me too! I thought pretty hard about this and couldn't come up with a better solution. Plus, I think we're going to need it later anyway if we want to be able to implement join with a timeout, at which point we will be waiting on it.

What about something like: ...

I considered this, but I think we need to set the flag after the PyThreadState has been destroyed. Otherwise, you could end up deadlocking in the following situation:

T0 joins T1.
T1 finishes. It has a thread-local with a finalizer that joins T1. The flag is set at this point, so we skip the self-join check and deadlock.

Since we have to set the flag after the PyThreadState has been destroyed, it's no longer safe to call Py_DECREF, so we can't use a ThreadHandleObject (or any Python object). We could create a ref-counted flag (or re-use one if it exists), but since we're already going to need the _PyEventRc anyway, I figured we'd just use it here.

This looks good to me.

@pitrou, would you please look this over? The motivation is to make thread joining thread-safe in the freethreaded build, as well as fix a race condition that can affect both the default and freethreaded build (described by @mpage in #114839)

In general this looks ok but I think this can be simplified, see below.

@pitrou - I think I've addressed all of your comments except for the question about using bool. A quick grep of the codebase reveals quite a few uses of bool (in central places like obmalloc.c and dictobject.c, among others). Even if there isn't explicit guidance, there's certainly precedent. I've also removed the detach() method. Would you please have another look?

@colesbury - This has changed a bit since you reviewed it. Would you please have another look? In particular, I'd like a sanity check that the use of relaxed atomics for accessing handle state is OK.

The failing macos-13 builds appear unrelated to this PR. They're failing with errors like:

error: call to undeclared function 'is_pad'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    if (py_is_pad(self->win)) {

A few minor comments below

@pitrou - I think I've addressed everything now. Would you please have a look?

@pitrou @gpshead - Did you want to take a look at this?

(review in progress, more to come)

This is interesting... our docs already claim that threads can be joined multiple times. So I wonder why this existing test logic was previously explicitly checking for an error here.

In this sense this change aligns with what our docs claim so a behavior change here could be seen as a bugfix. I do not expect anyone to be depending on subsequent join()s of a thread raising regardless.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again

@gpshead - Would you please have another look at this?

…aded builds (pythonGH-115190) Make `_thread.ThreadHandle` thread-safe in free-threaded builds We protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`. Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` block until it is their turn to execute or an earlier operation succeeds. Once an operation has been applied successfully all future operations complete immediately. The `join()` method is now idempotent. It may be called multiple times but the underlying OS thread will only be joined once. After `join()` succeeds, any future calls to `join()` will succeed immediately. The internal thread handle `detach()` method has been removed.

bedevere-app Bot mentioned this pull request Feb 8, 2024

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

Closed

mpage marked this pull request as ready for review February 9, 2024 00:55

bedevere-app Bot added the awaiting review label Feb 9, 2024

colesbury self-requested a review February 9, 2024 00:57

colesbury added the skip news label Feb 9, 2024

colesbury reviewed Feb 9, 2024

View reviewed changes

Comment thread Modules/_threadmodule.c Outdated

Comment thread Modules/_threadmodule.c Outdated

mpage added 4 commits February 9, 2024 11:50

Don't use PyOnceFlag post fork

e86dde8

Check for self-join outside of the once flag

9a61e20

Fix grammar and formatting

bcf66ce

Update comment.

7f9720d

mpage mentioned this pull request Feb 9, 2024

Add _PyOnceFlag_CallOnceTimed #115229

Closed

colesbury reviewed Feb 9, 2024

View reviewed changes

colesbury requested a review from pitrou February 9, 2024 23:32

colesbury approved these changes Feb 9, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 9, 2024

Account for handles being cleared after fork

092c2eb

pitrou reviewed Feb 12, 2024

View reviewed changes

mpage added 3 commits February 12, 2024 10:20

Move implementations of _PyEventRc functions out of header

77da31d

Track all states of the handle in the enum

321a4e0

Remove detach() method

cd0372c

colesbury reviewed Feb 13, 2024

View reviewed changes

Comment thread Modules/_threadmodule.c Outdated

Comment thread Modules/_threadmodule.c Outdated

Comment thread Modules/_threadmodule.c Outdated

colesbury mentioned this pull request Feb 13, 2024

Undeclared function 'is_pad' on macos-13 GitHub runners #115383

Closed

Don't need to atomically initialize state in ThreadHandle ctor

7badb2d

Use sequential consistency for operations on state

1fe27ce

colesbury requested a review from pitrou February 14, 2024 14:32

pitrou requested a review from gpshead February 14, 2024 16:47

Merge branch 'main' into pythongh-114271 -_thread-ThreadHandle

9ec6d23

gpshead requested changes Feb 26, 2024

View reviewed changes

bedevere-app Bot added awaiting changes and removed awaiting merge labels Feb 26, 2024

mpage added 2 commits February 26, 2024 15:18

Remove _join_lock attribute

a46b727

Add reference to more details for self-join test

61d9fa9

gpshead reviewed Feb 26, 2024

View reviewed changes

Comment thread Modules/_threadmodule.c

Comment thread Lib/test/test_thread.py

Comment thread Modules/_threadmodule.c

Comment thread Modules/_threadmodule.c

Comment thread Modules/_threadmodule.c

gpshead self-assigned this Feb 26, 2024

mpage added 4 commits February 26, 2024 16:13

Assign explicit values for ThreadHandle states

f6bfa08

Detail why non-atomic state access is safe in the destructor

1cc0f2e

Document the purpose of bootstate

3fbf119

Mark handles running before decrefing args

9d21444

mpage requested a review from gpshead February 27, 2024 23:54

gpshead reviewed Mar 1, 2024

View reviewed changes

Comment thread Modules/_threadmodule.c

gpshead approved these changes Mar 1, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting changes labels Mar 1, 2024

gpshead merged commit 9e88173 into python:main Mar 1, 2024

bedevere-app Bot removed the awaiting merge label Mar 1, 2024

                       with threading_helper.wait_threads_exit():
                           handle = thread.start_joinable_thread(task)
                           handle.join()
-                          with self.assertRaisesRegex(ValueError, "not joinable"):

gh-114271: Make `_thread.ThreadHandle` thread-safe in free-threaded builds by mpage · Pull Request #115190 · python/cpython · GitHub

Repository navigation

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

gh-114271: Make _thread.ThreadHandle thread-safe in free-threaded builds#115190gpshead merged 21 commits intopython:mainpython/cpython:mainfrom mpage:gh-114271-_thread-ThreadHandlempage/cpython:gh-114271-_thread-ThreadHandleCopy head branch name to clipboard

Conversation

mpage commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpage commented Feb 9, 2024

Uh oh!

Uh oh!

Uh oh!

mpage commented Feb 9, 2024

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

mpage commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpage commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpage commented Feb 13, 2024

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpage commented Feb 14, 2024

Uh oh!

mpage commented Feb 26, 2024

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

gpshead Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-app Bot commented Feb 26, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpage commented Feb 27, 2024

Uh oh!

mpage commented Feb 29, 2024

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gh-114271: Make _thread.ThreadHandle thread-safe in free-threaded builds#115190
gpshead merged 21 commits into
python:mainfrom
mpage:gh-114271-_thread-ThreadHandle

mpage commented Feb 8, 2024 •
edited
Loading

mpage commented Feb 9, 2024 •
edited
Loading

mpage commented Feb 13, 2024 •
edited
Loading