Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
gpshead merged 21 commits intopython:mainfrommpage:gh-114271-_thread-ThreadHandle
Mar 1, 2024

Conversation

@mpage
Copy link
Contributor

@mpagempage commentedFeb 8, 2024•
edited
Loading

We protect the mutable state ofThreadHandle using a_PyOnceFlag. Concurrent operations (i.e.join) onThreadHandle 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.

Thejoin() 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.

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

We protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`.Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` blockuntil it is their turn to execute or an earlier operation succeeds.Once an operation has been applied successfully all future operationscomplete immediately.The `join()` method is now idempotent. It may be called multiple timesbut 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 timesbut 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.
@mpagempage marked this pull request as ready for reviewFebruary 9, 2024 00:55
@mpage
Copy link
ContributorAuthor

@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 thetstate_lock stuff on top.

@mpage
Copy link
ContributorAuthor

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

colesbury reacted with thumbs up emoji

@mpagempage mentioned this pull requestFeb 9, 2024
Copy link
Contributor

@colesburycolesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 forThreadHandleObject because we are never waiting on the event -- we're just using it like an atomic flag.

What about something like:

  1. Makethread_is_exiting a plain variable that we use atomic operations to access, likeint thread_is_exiting (because we don't have atomic ops onbool currently)
  2. Pass theThreadHandleObject todo_start_new_thread instead of having another object with a separate lifetime.

@mpage
Copy link
ContributorAuthor

mpage commentedFeb 9, 2024•
edited
Loading

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 implementjoin 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 thePyThreadState has been destroyed. Otherwise, you could end up deadlocking in the following situation:

  1. T0 joins T1.
  2. 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 thePyThreadState has been destroyed, it's no longer safe to callPy_DECREF, so we can't use aThreadHandleObject (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.

colesbury reacted with thumbs up emoji

Copy link
Contributor

@colesburycolesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

Copy link
Member

@pitroupitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@mpage
Copy link
ContributorAuthor

mpage commentedFeb 13, 2024•
edited
Loading

@pitrou - I think I've addressed all of your comments except for the question about usingbool. A quick grep of the codebase reveals quite a few uses ofbool (in central places likeobmalloc.c anddictobject.c, among others). Even if there isn't explicit guidance, there's certainly precedent. I've also removed thedetach() 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.

@mpage
Copy link
ContributorAuthor

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)) {

Copy link
Contributor

@colesburycolesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

A few minor comments below

Edgarruiz856 reacted with heart emoji
@mpage
Copy link
ContributorAuthor

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

@mpage
Copy link
ContributorAuthor

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

Copy link
Member

@gpsheadgpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(review in progress, more to come)

withthreading_helper.wait_threads_exit():
handle=thread.start_joinable_thread(task)
handle.join()
withself.assertRaisesRegex(ValueError,"not joinable"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 notexpect anyone to be depending on subsequent join()s of a thread raising regardless.

@bedevere-app
Copy link

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 phraseI 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.

@gpsheadgpshead self-assigned thisFeb 26, 2024
@mpage
Copy link
ContributorAuthor

I have made the requested changes; please review again

@mpagempage requested a review fromgpsheadFebruary 27, 2024 23:54
@mpage
Copy link
ContributorAuthor

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

@gpsheadgpshead merged commit9e88173 intopython:mainMar 1, 2024
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull requestMar 4, 2024
…aded builds (pythonGH-115190)Make `_thread.ThreadHandle` thread-safe in free-threaded buildsWe protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`.Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` blockuntil it is their turn to execute or an earlier operation succeeds.Once an operation has been applied successfully all future operationscomplete immediately.The `join()` method is now idempotent. It may be called multiple timesbut 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.
adorilson pushed a commit to adorilson/cpython that referenced this pull requestMar 25, 2024
…aded builds (pythonGH-115190)Make `_thread.ThreadHandle` thread-safe in free-threaded buildsWe protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`.Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` blockuntil it is their turn to execute or an earlier operation succeeds.Once an operation has been applied successfully all future operationscomplete immediately.The `join()` method is now idempotent. It may be called multiple timesbut 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.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
…aded builds (pythonGH-115190)Make `_thread.ThreadHandle` thread-safe in free-threaded buildsWe protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`.Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` blockuntil it is their turn to execute or an earlier operation succeeds.Once an operation has been applied successfully all future operationscomplete immediately.The `join()` method is now idempotent. It may be called multiple timesbut 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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@colesburycolesburycolesbury approved these changes

@gpsheadgpsheadgpshead approved these changes

@pitroupitrouAwaiting requested review from pitrou

Assignees

@gpsheadgpshead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mpage@gpshead@colesbury@pitrou
[8]ページ先頭
©2009-2025 Movatter.jp