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-118727: Don't drop the GIL indrop_gil() unless the current thread holds it#118745

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
colesbury merged 10 commits intopython:mainfromswtaarrs:cpython-drop-gil
May 23, 2024

Conversation

swtaarrs
Copy link
Member

@swtaarrsswtaarrs commentedMay 7, 2024•
edited
Loading

drop_gil() assumes that its caller is attached, which means that the current thread holds the GIL if and only if the GIL is enabled, and the enabled-state of the GIL won't change. This isn't true, though, becausedetach_thread() calls_PyEval_ReleaseLock() after detaching and_PyThreadState_DeleteCurrent() calls it after removing the current thread from consideration for stop-the-world requests (effectively detaching it).

Fix this by remembering whether or not a thread acquired the GIL when it last attached, inPyThreadState._status.holds_gil, and check this indrop_gil() instead ofgil->enabled. As part of this, add an explicitthread_dying argument todrop_gil(), separating that concept from whether or not it's safe to dereferencetstate.

This fixes a crash intest_multiprocessing_pool_circular_import(), so I've reenabled it.

@swtaarrsswtaarrs removed the 3.13bugs and security fixes labelMay 7, 2024
@swtaarrs
Copy link
MemberAuthor

I realized while writing up an explanation of my changes that the commit I just pushed isn't quite right. I changeddrop_gil() to always take a non-NULLtstate by adding an explicitthread_dying argument. But inthis case,tstate can be a dangling pointer so it's actually important to not dereference it. I'll come up with a different approach.

@swtaarrsswtaarrs marked this pull request as draftMay 8, 2024 19:25
@swtaarrsswtaarrs marked this pull request as ready for reviewMay 8, 2024 20:58
@swtaarrs
Copy link
MemberAuthor

This should be ready now - I updated the description to reflect the changes.

@swtaarrsswtaarrs changed the titlegh-118727: Don't drop the GIL during thread deletion unless the current thread holds itgh-118727: Don't drop the GIL indrop_gil() unless the current thread holds itMay 8, 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.

Overall this LGTM. I think it's worth havingtstate->_status.holds_gil in the default build as well.

swtaarrs reacted with thumbs up emoji
@swtaarrsswtaarrs added the needs backport to 3.13bugs and security fixes labelMay 9, 2024
@swtaarrs
Copy link
MemberAuthor

(moved from the issue where I accidentally posted this)

test_importlib got stuck in thetsan build again, which this should've fixed. I'll see what I can figure out.

ericsnowcurrently reacted with thumbs up emoji

@ericsnowcurrently
Copy link
Member

Does applying@suppress_immortalization() make a difference. I'm not suggesting that as a solution (there's probably a deeper issue to solve), but whether or not the decorator makes a difference might be helpful to know.

swtaarrs reacted with thumbs up emoji

@colesbury
Copy link
Contributor

@swtaarrs - I observed a hang when runningpool_in_threads.py locally with this PR. (It may or may not be the same as the TSan hang).

I think there's an issue withFORCE_SWITCHING and the dynamic enabling of the GIL:

  • A thread disables (and releases) the GIL via_PyEval_DisableGIL. If_PY_GIL_DROP_REQUEST_BIT is set, it waits in theFORCE_SWITCHING section until another thread acquires the GIL
  • A second thread acquires the GIL. However, it sees that the GIL is now disabled and releases the GILwithout notifying the first thread
swtaarrs reacted with thumbs up emoji

@swtaarrs
Copy link
MemberAuthor

I think there's an issue withFORCE_SWITCHING and the dynamic enabling of the GIL

Thanks, good catch. I should probably pulldrop_gil_impl() back out for use by_PyEval_DisableGIL() then. TheFORCE_SWITCHING code doesn't apply once the GIL is disabled, and nothing else indrop_gil() applies to_PyEval_DisableGIL() either. I'll also take another look at everything at the end oftake_gil() that's skipped in this case.

@swtaarrs
Copy link
MemberAuthor

The commit I just pushed should address theFORCE_SWITCHING issue by reverting mydrop_gil_impl() change. I also added a line to clear_PY_GIL_DROP_REQUEST_BIT when disabling the GIL, so that thread doesn't keep hitting its eval_breaker once it resumes executing.

I'm running the tsan tests in a loop locally to see if it hangs again.

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.

LGTM

@colesbury
Copy link
Contributor

@ericsnowcurrently - would you like to re-review this or should I go ahead and merge it?

@ericsnowcurrently
Copy link
Member

I'll take a quick look tomorrow.

colesbury reacted with thumbs up emoji

@swtaarrs
Copy link
MemberAuthor

@ericsnowcurrently will you have time to take a look today?

@ericsnowcurrently
Copy link
Member

Sorry for the delay. I don't have any objections. If you both are confident about the solution then I'm on board. 😄

swtaarrs reacted with thumbs up emoji

@colesbury
Copy link
Contributor

Great! As a heads up, there is a different bug that affects the same test case (#119369).

@colesburycolesbury merged commitbe1dfcc intopython:mainMay 23, 2024
@miss-islington-app
Copy link

Thanks@swtaarrs for the PR, and@colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestMay 23, 2024
…t thread holds it (pythonGH-118745)`drop_gil()` assumes that its caller is attached, which means that the currentthread holds the GIL if and only if the GIL is enabled, and the enabled-stateof the GIL won't change. This isn't true, though, because `detach_thread()`calls `_PyEval_ReleaseLock()` after detaching and`_PyThreadState_DeleteCurrent()` calls it after removing the current threadfrom consideration for stop-the-world requests (effectively detaching it).Fix this by remembering whether or not a thread acquired the GIL when it lastattached, in `PyThreadState._status.holds_gil`, and check this in `drop_gil()`instead of `gil->enabled`.This fixes a crash in `test_multiprocessing_pool_circular_import()`, so I'vereenabled it.(cherry picked from commitbe1dfcc)Co-authored-by: Brett Simmers <swtaarrs@users.noreply.github.com>
@bedevere-app
Copy link

GH-119474 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelMay 23, 2024
colesbury pushed a commit that referenced this pull requestMay 23, 2024
…nt thread holds it (GH-118745) (#119474)`drop_gil()` assumes that its caller is attached, which means that the currentthread holds the GIL if and only if the GIL is enabled, and the enabled-stateof the GIL won't change. This isn't true, though, because `detach_thread()`calls `_PyEval_ReleaseLock()` after detaching and`_PyThreadState_DeleteCurrent()` calls it after removing the current threadfrom consideration for stop-the-world requests (effectively detaching it).Fix this by remembering whether or not a thread acquired the GIL when it lastattached, in `PyThreadState._status.holds_gil`, and check this in `drop_gil()`instead of `gil->enabled`.This fixes a crash in `test_multiprocessing_pool_circular_import()`, so I'vereenabled it.(cherry picked from commitbe1dfcc)Co-authored-by: Brett Simmers <swtaarrs@users.noreply.github.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull requestJul 17, 2024
…t thread holds it (python#118745)`drop_gil()` assumes that its caller is attached, which means that the currentthread holds the GIL if and only if the GIL is enabled, and the enabled-stateof the GIL won't change. This isn't true, though, because `detach_thread()`calls `_PyEval_ReleaseLock()` after detaching and`_PyThreadState_DeleteCurrent()` calls it after removing the current threadfrom consideration for stop-the-world requests (effectively detaching it).Fix this by remembering whether or not a thread acquired the GIL when it lastattached, in `PyThreadState._status.holds_gil`, and check this in `drop_gil()`instead of `gil->enabled`.This fixes a crash in `test_multiprocessing_pool_circular_import()`, so I'vereenabled it.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@colesburycolesburycolesbury approved these changes

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

Assignees
No one assigned
Labels
interpreter-core(Objects, Python, Grammar, and Parser dirs)skip newstopic-free-threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@swtaarrs@ericsnowcurrently@colesbury
[8]ページ先頭
©2009-2025 Movatter.jp