gh-87135: Hang non-main threads that attempt to acquire the GIL during finalization by jbms · Pull Request #105805 · python/cpython · GitHub
Skip to content

gh-87135: Hang non-main threads that attempt to acquire the GIL during finalization - #105805

Merged
gpshead merged 17 commits into
python:mainfrom
jbms:new-gil-hang
Oct 2, 2024
Merged

gh-87135: Hang non-main threads that attempt to acquire the GIL during finalization#105805
gpshead merged 17 commits into
python:mainfrom
jbms:new-gil-hang

Conversation

@jbms

@jbms jbms commented Jun 14, 2023 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

This splits off the change from #28525 to hang threads that attempt to acquire the GIL during interpreter shutdown, but does not introduce any new public APIs.

📚 Documentation preview 📚: https://cpython-previews--105805.org.readthedocs.build/

@bedevere-bot bedevere-bot mentioned this pull request Jun 14, 2023
Closed
@jbms jbms mentioned this pull request Jun 14, 2023
Closed
@ericsnowcurrently ericsnowcurrently mentioned this pull request Aug 16, 2023
Closed
2 tasks
vstinner
vstinner reviewed Aug 16, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, as I wrote in issue #87135, I dislike switching to this behavior by default :-(

I would prefer to keep the current behavior by default, but give the ability to hang for people impacted by pthread_exit() issues.

Comment thread Python/thread_pthread.h
@ericsnowcurrently ericsnowcurrently mentioned this pull request Sep 23, 2023
Closed
@gpshead gpshead self-assigned this Sep 26, 2024
@ericsnowcurrently ericsnowcurrently mentioned this pull request Sep 26, 2024
Open
@gpshead

gpshead commented Sep 26, 2024

Copy link
Copy Markdown
Member

@gpshead

gpshead commented Sep 26, 2024

Copy link
Copy Markdown
Member

@colesbury and/or @mpage, and @ericsnowcurrently - can you take a look at this PR? (FWIW, Eric's already gone over it with my on screen at the core dev sprint here)

@gpshead gpshead mentioned this pull request Sep 27, 2024
Closed
@mpage

mpage commented Sep 27, 2024

Copy link
Copy Markdown
Contributor

Blocking daemon threads after the VM has been finalized seems less error prone than forcing the threads to exit. I believe this is also how the JVM handles the same situation, so there's some precedence for this approach.

While we're here, I think this might be a good time to also fix the issues with threads referencing their PyThreadStates after they have been freed. This would allow us to avoid having to introduce new APIs to put threads to sleep. Instead, threads could block on a lock in their PyThreadState that is never released (also what the JVM does). I believe @colesbury suggested refcounting PyThreadStates as a solution to the PyThreadState lifetime problem.

vstinner
vstinner reviewed Sep 27, 2024
View reviewed changes
Comment thread Modules/_testcapimodule.c Outdated
Comment thread Modules/_testcapimodule.c Outdated
colesbury
colesbury reviewed Sep 30, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with what @gpshead and @mpage wrote. I think this is the right choice for default behavior.

Comment thread Doc/c-api/init.rst Outdated
Comment thread Include/pythread.h Outdated
@gpshead gpshead mentioned this pull request Oct 2, 2024
Closed
@gpshead
Skip the new test under TSAN (and MSAN).
ac3d2be 
Linking to the filed pre-existing known issue.
@gpshead

gpshead commented Oct 2, 2024

Copy link
Copy Markdown
Member

While we're here, I think this might be a good time to also fix the issues with threads referencing their PyThreadStates after they have been freed. This would allow us to avoid having to introduce new APIs to put threads to sleep. Instead, threads could block on a lock in their PyThreadState that is never released (also what the JVM does). I believe @colesbury suggested refcounting PyThreadStates as a solution to the PyThreadState lifetime problem.

That goes beyond the scope of what I want to accomplish with this PR - which I believe might mostly be a candidate for backporting as a bug fix (I'll see what the RMs think).

I think it is a good followup issue though - #124878 for starters but that might turn into a couple of different things to chase down as well.

@gpshead
gpshead merged commit 8cc5aa4 into python:main Oct 2, 2024
@gpshead

gpshead commented Oct 2, 2024

Copy link
Copy Markdown
Member

@Yhg1s what's your opinion on the viability of backporting this as a bugfix to 3.13 and 3.12 patch releases? People who actually encounter this in the form of strange crashes from threads during shutdown have been wanting it for years. I wouldn't call it common, just a thorn in some classes of applications side.

(In terms of impact I'd elide marking the deprecated C API as deprecated in the backports - otherwise I expect most of this to apply relatively easily given that it was originally authored on a 3.12 or earlier base and I only merged it forward to current main last week)

@gpshead gpshead added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Oct 28, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 28, 2024

Copy link
Copy Markdown

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

@miss-islington-app

miss-islington-app Bot commented Oct 28, 2024

Copy link
Copy Markdown

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

@miss-islington-app

miss-islington-app Bot commented Oct 28, 2024

Copy link
Copy Markdown

Sorry, @jbms and @gpshead, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8cc5aa47ee464ddfd8da5461edecf4a5c72df2ff 3.12

@miss-islington-app

miss-islington-app Bot commented Oct 28, 2024

Copy link
Copy Markdown

Sorry, @jbms and @gpshead, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8cc5aa47ee464ddfd8da5461edecf4a5c72df2ff 3.13

@gpshead

gpshead commented Oct 28, 2024

Copy link
Copy Markdown
Member

I expected the backport automation to fail and need edits regardless even if it hadn't. I'll be taking care of them. :)

@smonkewitz smonkewitz mentioned this pull request Dec 9, 2024
Open
2 tasks
@hugovk

hugovk commented Feb 21, 2025

Copy link
Copy Markdown
Member

Triage: @gpshead reminder about the backports :)

@hugovk hugovk removed the needs backport to 3.12 only security fixes label Apr 9, 2025
@serhiy-storchaka

serhiy-storchaka commented Aug 10, 2025

Copy link
Copy Markdown
Member

Please don't forget about backport. @gpshead

gpshead added a commit to gpshead/cpython that referenced this pull request Aug 15, 2025
@jbms @gpshead
[3.13] pythongh-87135: Hang non-main threads that attempt to acquire …
f3e301d 
…the GIL during finalization (pythonGH-105805)

Instead of surprise crashes and memory corruption, we now hang threads that attempt to re-enter the Python interpreter after Python runtime finalization has started. These are typically daemon threads (our long standing mis-feature) but could also be threads spawned by extension modules that then try to call into Python. This marks the `PyThread_exit_thread` public C API as deprecated as there is no plausible safe way to accomplish that on any supported platform in the face of things like C++ code with finalizers anywhere on a thread's stack. Doing this was the least bad option.

(cherry picked from commit 8cc5aa4)

Co-authored-by: Jeremy Maitin-Shepard <jeremy@jeremyms.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
@bedevere-app

bedevere-app Bot commented Aug 15, 2025

Copy link
Copy Markdown

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Aug 15, 2025
gpshead added a commit that referenced this pull request Aug 17, 2025
@gpshead @jbms
[3.13] gh-87135: Hang non-main threads that attempt to acquire the GI…
9face21 
…L during finalization (GH-105805) (GH-137827)

* [3.13] gh-87135: Hang non-main threads that attempt to acquire the GIL during finalization (GH-105805)

Instead of surprise crashes and memory corruption, we now hang threads that attempt to re-enter the Python interpreter after Python runtime finalization has started. These are typically daemon threads (our long standing mis-feature) but could also be threads spawned by extension modules that then try to call into Python. This marks the `PyThread_exit_thread` public C API as deprecated as there is no plausible safe way to accomplish that on any supported platform in the face of things like C++ code with finalizers anywhere on a thread's stack. Doing this was the least bad option.

(cherry picked from commit 8cc5aa4)

Co-authored-by: Jeremy Maitin-Shepard <jeremy@jeremyms.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>

* state "3.13.7 and earlier"
* backport: do not add the deprecated marker
* fix Py_IsFinalizing doc ref

---------

Co-authored-by: Jeremy Maitin-Shepard <jeremy@jeremyms.com>
@bedevere-bot

bedevere-bot commented Aug 17, 2025

Copy link
Copy Markdown

@scw scw mentioned this pull request Nov 12, 2025
Open
@KowalskiThomas KowalskiThomas mentioned this pull request Sep 4, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@colesbury colesbury colesbury left review comments
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@gpshead gpshead Awaiting requested review from gpshead

Assignees

@gpshead gpshead

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jbms @gpshead @mpage @hugovk @serhiy-storchaka @bedevere-bot @vstinner @colesbury