gh-114271: Make _thread.ThreadHandle thread-safe in free-threaded builds
#115190
Conversation
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 |
|
@colesbury - This should be good to go. Would you please have another look? |
There was a problem hiding this comment.
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_exitinga plain variable that we use atomic operations to access, likeint thread_is_exiting(because we don't have atomic ops onboolcurrently) - Pass the
ThreadHandleObjecttodo_start_new_threadinstead of having another object with a separate lifetime.
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
I considered this, but I think we need to set the flag after the
Since we have to set the flag after the |
|
@pitrou - I think I've addressed all of your comments except for the question about using @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: |
|
@pitrou - I think I've addressed everything now. Would you please have a look? |
| @@ -189,8 +189,8 @@ def task(): | |||
| with threading_helper.wait_threads_exit(): | |||
| handle = thread.start_joinable_thread(task) | |||
| handle.join() | |||
| with self.assertRaisesRegex(ValueError, "not joinable"): | |||
There was a problem hiding this comment.
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 |
|
@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.
…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.
…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.
We protect the mutable state of
ThreadHandleusing a_PyOnceFlag. Concurrent operations (i.e.join) onThreadHandleblock 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. Afterjoin()succeeds, any future calls tojoin()will succeed immediately.The
detach()method has been removed; nothing was using it._threadmodule.cthread-safe in--disable-gilbuilds #114271