gh-118727: Don't drop the GIL in `drop_gil()` unless the current thread holds it #118745

swtaarrs · 2024-05-07T23:19:46Z

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, because detach_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, in PyThreadState._status.holds_gil, and check this in drop_gil() instead of gil->enabled. As part of this, add an explicit thread_dying argument to drop_gil(), separating that concept from whether or not it's safe to dereference tstate.

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

Issue: Crash in PyThreadState_DeleteCurrent: drop_gil: GIL is not locked (free-threading) #118727

…olds it

Python/pystate.c

…priate

swtaarrs · 2024-05-08T19:24:47Z

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

swtaarrs · 2024-05-08T21:00:33Z

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

colesbury

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

Python/pystate.c

Include/cpython/pystate.h

Python/ceval_gil.c

swtaarrs · 2024-05-09T23:42:44Z

(moved from the issue where I accidentally posted this)

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

ericsnowcurrently · 2024-05-10T16:21:39Z

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.

colesbury · 2024-05-10T18:38:03Z

@swtaarrs - I observed a hang when running pool_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 with FORCE_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 the FORCE_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 GIL without notifying the first thread

swtaarrs · 2024-05-13T15:43:04Z

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

Thanks, good catch. I should probably pull drop_gil_impl() back out for use by _PyEval_DisableGIL() then. The FORCE_SWITCHING code doesn't apply once the GIL is disabled, and nothing else in drop_gil() applies to _PyEval_DisableGIL() either. I'll also take another look at everything at the end of take_gil() that's skipped in this case.

swtaarrs · 2024-05-13T18:52:36Z

The commit I just pushed should address the FORCE_SWITCHING issue by reverting my drop_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.

colesbury

LGTM

colesbury · 2024-05-21T22:13:52Z

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

ericsnowcurrently · 2024-05-21T22:25:51Z

I'll take a quick look tomorrow.

swtaarrs · 2024-05-23T18:06:11Z

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

ericsnowcurrently · 2024-05-23T18:10:26Z

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

colesbury · 2024-05-23T18:53:43Z

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

miss-islington-app · 2024-05-23T20:59:38Z

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

…t thread holds it (pythonGH-118745) `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, because `detach_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, 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've reenabled it. (cherry picked from commit be1dfcc) Co-authored-by: Brett Simmers <[email protected]>

bedevere-app · 2024-05-23T20:59:50Z

GH-119474 is a backport of this pull request to the 3.13 branch.

…nt thread holds it (GH-118745) (#119474) `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, because `detach_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, 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've reenabled it. (cherry picked from commit be1dfcc) Co-authored-by: Brett Simmers <[email protected]>

…t thread holds it (python#118745) `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, because `detach_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, 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've reenabled it.

Don't drop the GIL during thread deletion unless the current thread h…

1d21956

…olds it

swtaarrs added skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-free-threading labels May 7, 2024

swtaarrs requested a review from colesbury May 7, 2024 23:19

bedevere-app bot mentioned this pull request May 7, 2024

Crash in PyThreadState_DeleteCurrent: drop_gil: GIL is not locked (free-threading) #118727

Closed

swtaarrs marked this pull request as ready for review May 7, 2024 23:30

swtaarrs requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners May 7, 2024 23:30

bedevere-app bot added the awaiting review label May 7, 2024

swtaarrs added the 3.13 bugs and security fixes label May 7, 2024

colesbury reviewed May 7, 2024

View reviewed changes

Python/pystate.c Outdated Show resolved Hide resolved

swtaarrs removed the 3.13 bugs and security fixes label May 7, 2024

ericsnowcurrently removed request for brettcannon, warsaw and ncoghlan May 8, 2024 14:32

Store holds_gil flag on PyThreadState instead, and use it where appro…

b91a1d5

…priate

swtaarrs marked this pull request as draft May 8, 2024 19:25

bedevere-app bot removed the awaiting review label May 8, 2024

swtaarrs added 2 commits May 8, 2024 13:19

Merge remote-tracking branch 'upstream/main' into cpython-drop-gil

402ab90

Account for tstate == NULL case

29c853d

swtaarrs marked this pull request as ready for review May 8, 2024 20:58

bedevere-app bot added the awaiting review label May 8, 2024

swtaarrs changed the title ~~gh-118727: Don't drop the GIL during thread deletion unless the current thread holds it~~ gh-118727: Don't drop the GIL in drop_gil() unless the current thread holds it May 8, 2024

colesbury reviewed May 8, 2024

View reviewed changes

Python/pystate.c Outdated Show resolved Hide resolved

Include/cpython/pystate.h Outdated Show resolved Hide resolved

Python/ceval_gil.c Outdated Show resolved Hide resolved

Always track holds_gil, related cleanup

694926f

swtaarrs added the needs backport to 3.13 bugs and security fixes label May 9, 2024

swtaarrs added 2 commits May 9, 2024 13:26

Rename thread_dying to final_release

d28bf13

Merge remote-tracking branch 'upstream/main' into cpython-drop-gil

43f64d7

swtaarrs added 2 commits May 13, 2024 11:29

Restore drop_gil_impl(), clear GIL drop request when disabling GIL

d056a46

Merge remote-tracking branch 'upstream/main' into cpython-drop-gil

f32a503

colesbury approved these changes May 21, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels May 21, 2024

colesbury requested a review from ericsnowcurrently May 21, 2024 22:13

colesbury mentioned this pull request May 21, 2024

Deadlock in pool_in_threads.py (test_multiprocessing_pool_circular_import) in free-threaded build #119369

Closed

colesbury merged commit be1dfcc into python:main May 23, 2024

bedevere-app bot removed the awaiting merge label May 23, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 23, 2024

JelleZijlstra mentioned this pull request May 28, 2024

backport a9a74da 3.13 #119642

Closed

colesbury mentioned this pull request Jun 3, 2024

"thread sanitizer" (tsan) CI job is hanging #118729

Closed

Uh oh!

gh-118727: Don't drop the GIL in drop_gil() unless the current thread holds it #118745

gh-118727: Don't drop the GIL in drop_gil() unless the current thread holds it #118745

Uh oh!

Conversation

swtaarrs commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

swtaarrs commented May 8, 2024

Uh oh!

swtaarrs commented May 8, 2024

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swtaarrs commented May 9, 2024

Uh oh!

ericsnowcurrently commented May 10, 2024

Uh oh!

colesbury commented May 10, 2024

Uh oh!

swtaarrs commented May 13, 2024

Uh oh!

swtaarrs commented May 13, 2024

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

colesbury commented May 21, 2024

Uh oh!

ericsnowcurrently commented May 21, 2024

Uh oh!

swtaarrs commented May 23, 2024

Uh oh!

ericsnowcurrently commented May 23, 2024

Uh oh!

colesbury commented May 23, 2024

Uh oh!

miss-islington-app bot commented May 23, 2024

Uh oh!

bedevere-app bot commented May 23, 2024

Uh oh!

Uh oh!

gh-118727: Don't drop the GIL in `drop_gil()` unless the current thread holds it #118745

gh-118727: Don't drop the GIL in `drop_gil()` unless the current thread holds it #118745

swtaarrs commented May 7, 2024 •

edited

Loading