Skip to content

gh-136003: Execute pre-finalization callbacks in a loop - #136004

Merged
ZeroIntensity merged 33 commits into
python:mainfrom
ZeroIntensity:fix-circular-finalization
Sep 18, 2025
Merged

gh-136003: Execute pre-finalization callbacks in a loop#136004
ZeroIntensity merged 33 commits into
python:mainfrom
ZeroIntensity:fix-circular-finalization

Conversation

@ZeroIntensity

@ZeroIntensityZeroIntensity commented Jun 26, 2025•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

@bedevere-appbedevere-appBot mentioned this pull request Jun 26, 2025
Closed
vstinner
vstinner reviewed Jul 3, 2025
View reviewed changes
Comment threadPython/pylifecycle.c
/* Each of these functions can start one another, e.g. a pending call
* could start a thread or vice versa. To ensure that we properly clean
* call everything, we run these in a loop until none of them run anything. */
for (;;) {

@vstinnervstinnerJul 3, 2025

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.

Would it make sense to add an arbitrary limit to detect infinite loop? For example, log an error after 16 attemps.

@ZeroIntensityZeroIntensityJul 3, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Well, it would prevent deadlocks in rare cases, but it would cause crashes in other equally rare cases. Maybe it would be better to emit a fatal error when there are too many iterations?

sobolevn
sobolevn reviewed Jul 3, 2025
View reviewed changes
Comment threadLib/test/test_atexit.py Outdated
ericsnowcurrently
ericsnowcurrently requested changes Jul 3, 2025
View reviewed changes

@ericsnowcurrentlyericsnowcurrently 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.

Overall, good work. 😄

I'm pretty sure there's a race we need to deal with which will require a variety of changes to this PR.

Comment threadLib/test/test_atexit.py
Comment threadLib/test/test_capi/test_misc.py
Comment threadPython/pylifecycle.c Outdated
@bedevere-app

bedevere-appBot commented Jul 3, 2025

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@ZeroIntensity
Use an RW lock instead of a counter.
859070f
@ZeroIntensity
Merge branch 'main' ofhttps://github.com/python/cpythoninto fix-cir…
37098a0
…cular-finalization
@ZeroIntensity
Remove more counters.
8a1aa13
@ZeroIntensity
Remove old artifacts (again).
cbcd552
@ZeroIntensity
Final time removing artifacts.
54613a1
@ZeroIntensity
(I lied)
9ccdb5f
@ZeroIntensity
Atomically check if there are threads, atexit callbacks, or pending c…
9cd75b7
…alls.
@ZeroIntensity
Remove stray newline change.
1360059
@ZeroIntensity
Add a test for atexit with subinterpreters.
475538a
@ZeroIntensity
Check for os.pipe() in the test.
a794188
@ZeroIntensity
Rely on stop-the-world and the GIL instead of a dedicated RW mutex.
2dda7a4
This solution is much simpler, but will perform a little bit worse.
Instead of using a dedicated lock on the interpreter state, we can
simply use stop-the-world (and the GIL on the default build) to ensure that
no other threads can create pre-finalization callbacks concurrently.
@ZeroIntensityZeroIntensity added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 17, 2025
@bedevere-bot

This comment was marked as outdated.

Sign in to view
@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 17, 2025
@ZeroIntensity

ZeroIntensity commented Sep 17, 2025

Copy link
Copy Markdown
MemberAuthor

Nevermind, I broke FT when I fixed the merge conflicts. Ugh.

@ZeroIntensityZeroIntensity added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 17, 2025
@bedevere-bot

bedevere-bot commented Sep 17, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit e202848 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136004%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 17, 2025
@ZeroIntensity

ZeroIntensity commented Sep 17, 2025

Copy link
Copy Markdown
MemberAuthor

@ericsnowcurrently I'll hold off on merging until we talk tomorrow. We might want to extend the check to subinterpreters, and then disallow creating new interpreters during finalization.

@ZeroIntensityZeroIntensity mentioned this pull request Sep 18, 2025
Closed
@ZeroIntensityZeroIntensity mentioned this pull request Sep 18, 2025
Merged
@ZeroIntensity
ZeroIntensity merged commit 2191497 into python:mainSep 18, 2025
45 checks passed
@ZeroIntensity
ZeroIntensity deleted the fix-circular-finalization branch September 18, 2025 12:29
@github-project-automationgithub-project-automationBot moved this from Todo to Done in SprintSep 18, 2025
@ZeroIntensityZeroIntensity mentioned this pull request Sep 18, 2025
Merged
ZeroIntensity added a commit that referenced this pull request Sep 18, 2025
@ZeroIntensity
gh-136003: Skip non-daemon threads when exceptions occur during final…
3eec897
…ization (GH-139129)
During finalization, we need to mark all non-daemon threads as daemon to quickly shut down threads when sending CTRL^C to the process. This was a minor regression from GH-136004.
@encukouencukou mentioned this pull request Sep 22, 2025
Merged
encukou added a commit that referenced this pull request Sep 22, 2025
@encukou
gh-136003: Close file descriptors in test (GH-139225)
61e54e5
This fixes file descriptor leaks introduced in GH-136004
@yihong0618yihong0618 mentioned this pull request Oct 15, 2025
Merged
ZeroIntensity added a commit that referenced this pull request Oct 15, 2025
@yihong0618@ZeroIntensity@vstinner
gh-140080: Clearatexitcallbacks when memory allocation fails duri…
a05aece
…ng finalization (GH-140103)
This fixes a regression introduced by GH-136004, in which finalization would hang while executing atexit handlers if the system was out of memory.
---------
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
@yihong0618@ZeroIntensity
@vstinner@StanFromIreland
pythongh-140080: Clearatexitcallbacks when memory allocation fail…
dec4e9d
…s during finalization (pythonGH-140103)
This fixes a regression introduced by pythonGH-136004, in which finalization would hang while executing atexit handlers if the system was out of memory.
---------
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
@ZeroIntensityZeroIntensity mentioned this pull request May 4, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments
@ericsnowcurrentlyericsnowcurrentlyericsnowcurrently approved these changes
@sobolevnsobolevnsobolevn left review comments
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees

No one assigned

Labels

sprint

Projects

Sprint
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ZeroIntensity@bedevere-bot@vstinner@ericsnowcurrently@sobolevn