Skip to content

gh-115035: Mark ThreadHandles as non-joinable earlier after forking #115042

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 6, 2024
Merged

gh-115035: Mark ThreadHandles as non-joinable earlier after forking #115042

merged 2 commits into from
Feb 6, 2024

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Feb 5, 2024 •
edited by bedevere-app bot
Loading

This marks dead ThreadHandles as non-joinable earlier in PyOS_AfterFork_Child() before we execute any Python code. The handles are stored in a global linked list in _PyRuntimeState because fork() affects the entire process.

@colesbury
pythongh-115035: Mark ThreadHandles as non-joinable earlier
8130173 
This marks dead ThreadHandles as non-joinable earlier in
`PyOS_AfterFork_Child()` before we execute any Python code. The handles
are stored in a global linked list in `_PyRuntimeState` because `fork()`
affects the entire process.
@bedevere-app bedevere-app bot mentioned this pull request Feb 5, 2024
Closed
@colesbury colesbury changed the title gh-115035: Mark ThreadHandles as non-joinable earlier gh-115035: Mark ThreadHandles as non-joinable earlier after forking Feb 5, 2024
@colesbury
Copy link
Contributor Author

This doesn't call PyThread_update_thread_after_fork(). As far as I can tell, that call did not have any effect: we determine the current thread using PyThread_get_thread_ident_ex() (via get_ident) 1 so there's no way we could have a thread with a stale identifier. And if we don't find a matching thread, we create a new _MainThread() with the correct current identifier (and no handle).

Footnotes

  1. https://github.com/python/cpython/blob/c32bae52904723d99e1f98e2ef54570268d86467/Lib/threading.py#L1725-L1731 ↩

@pitrou
Copy link
Member

pitrou commented Feb 6, 2024

As far as I can tell, that call did not have any effect: we determine the current thread using PyThread_get_thread_ident_ex() (via get_ident) 1 so there's no way we could have a thread with a stale identifier.

That's a good point, but then we can probably remove the PyThread_update_thread_after_fork function?

pitrou
pitrou reviewed Feb 6, 2024
View reviewed changes
@colesbury colesbury requested a review from pitrou February 6, 2024 15:51
pitrou
pitrou approved these changes Feb 6, 2024
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Neat, thank you @colesbury !

@pitrou
Copy link
Member

pitrou commented Feb 6, 2024

Note: you're a core developer now ;-)

@colesbury colesbury merged commit b6228b5 into python:main Feb 6, 2024
@colesbury colesbury deleted the gh-115035-thread-handle branch February 6, 2024 19:45
@gpshead
Copy link
Member

gpshead commented Feb 7, 2024

nice refactoring, thanks! happy first-ish merge!

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@colesbury @fsc-eriker
pythongh-115035: Mark ThreadHandles as non-joinable earlier after for…
dffd892 
…king (python#115042)

This marks dead ThreadHandles as non-joinable earlier in
`PyOS_AfterFork_Child()` before we execute any Python code. The handles
are stored in a global linked list in `_PyRuntimeState` because `fork()`
affects the entire process.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitrou pitrou pitrou approved these changes

@gpshead gpshead Awaiting requested review from gpshead

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@colesbury @pitrou @gpshead