GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-128639: Don't assume one thread in subinterpreter finalization by ZeroIntensity · Pull Request #128640 · python/cpython

Conversation

ZeroIntensity commented Jan 8, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Incidentally, this also fixed the warning not showing up if a subinterpreter wasn't cleaned up via _interpreters.destroy. I had to update some of the tests as a result.

Issue: Subinterpreters don't properly clean up threads #128639

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ZeroIntensity added 2 commits

January 8, 2025 12:26


          Fix subinterpreter cleanup.

025cc17


          Add and fix tests.

eea3ba9

ZeroIntensity added the topic-subinterpreters label

Jan 8, 2025

ZeroIntensity requested a review from ericsnowcurrently as a code owner

January 8, 2025 17:51

bedevere-app Bot mentioned this pull request

Jan 8, 2025

Subinterpreters don't properly clean up threads #128639

Closed

bedevere-app Bot added the awaiting review label

Jan 8, 2025

ZeroIntensity added 3 commits

January 8, 2025 12:52


          Add blurb

149e0cf


          Fix threading tests.

1761c37


          Fix test_embed.

8febfbf

ZeroIntensity commented

Jan 8, 2025

View reviewed changes

Comment thread

Programs/_testembed.c

		Py_NewInterpreter();
		Py_NewInterpreter();
		Py_NewInterpreter();
		PyThreadState *tstate = PyThreadState_Get();

ZeroIntensity Jan 8, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this test working before was a fluke. Py_NewInterpreter changes the thread state, and I don't think we support calling Py_Finalize from a subinterpreter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ericsnowcurrently May 19, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's probably okay but your change helps future-proof the test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ZeroIntensity added 2 commits

January 8, 2025 14:53


          Remove buggy test.

5a73ce1


          Skip flaky test.

a7cc3fe

ZeroIntensity mentioned this pull request

Jan 11, 2025

Multithreaded subinterpreters can be running during finalization #126016

Closed

ZeroIntensity mentioned this pull request

Feb 4, 2025

Fail PyGILState_Ensure() If Finalizing? #124622

Open

ZeroIntensity added 4 commits

February 3, 2025 20:58


          Add an assertion.

7c44c0b


          Remove stray newline.

6551bfa


          Add another assertion.

f19e28d


          Merge branch 'main' into subinterpreter-thread-shutdown

02da72a

vstinner commented Feb 25, 2025

Copy link

Copy Markdown

Member

cc @ericsnowcurrently

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sergey-miryanov reviewed

Mar 15, 2025

View reviewed changes

Comment thread

Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity added 2 commits

March 28, 2025 16:08


          Merge branch 'main' of https://github.com/python/cpython into subinte…

a759540

…rpreter-thread-shutdown


          Use new is_after_fork parameter

76e4ec9

ZeroIntensity mentioned this pull request

May 17, 2025

Cannot safely Py_EndInterpreter in 3.14b1 #134144

Closed

ericsnowcurrently mentioned this pull request

May 17, 2025

gh-134144: Fix use-after-free in zapthreads() #134145

Merged

ericsnowcurrently requested changes

May 19, 2025

View reviewed changes

ericsnowcurrently left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

mostly LGTM

There are a few things that might make more sense in a separate PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/pylifecycle.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Programs/_testembed.c

		Py_NewInterpreter();
		Py_NewInterpreter();
		Py_NewInterpreter();
		PyThreadState *tstate = PyThreadState_Get();

ericsnowcurrently May 19, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's probably okay but your change helps future-proof the test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_threading.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_interpreters/test_lifecycle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_interpreters/test_api.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the awaiting review label

May 19, 2025

bedevere-app Bot commented May 19, 2025

Copy link

Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot added the awaiting changes label

May 19, 2025

ZeroIntensity and others added 2 commits

May 19, 2025 11:16


          Apply suggestions from code review

Co-authored-by: Eric Snow <[email protected]>


          Move call to finalize_subinterpreters()

d61099e

ZeroIntensity added 2 commits

May 19, 2025 11:34


          Unskip the threading test.

09851dc


          Add a comment for the test.

b223396

ericsnowcurrently approved these changes

May 19, 2025

View reviewed changes

ericsnowcurrently left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Thanks for doing this!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting changes labels

May 19, 2025

ericsnowcurrently merged commit 9859791 into python:main

May 19, 2025

bedevere-app Bot removed the awaiting merge label

May 19, 2025

ericsnowcurrently commented May 19, 2025

Copy link

Copy Markdown

Member

@hugovk, is this something you'd be okay with us backporting to 3.14?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity deleted the subinterpreter-thread-shutdown branch

May 19, 2025 16:24

hugovk commented May 19, 2025

Copy link

Copy Markdown

Member

Yep, crash bugfixes can be backported :)

ZeroIntensity reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity added the needs backport to 3.14 bugs and security fixes label

May 19, 2025

miss-islington-app Bot commented May 19, 2025

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

May 19, 2025


          pythongh-128639: Don't assume one thread in subinterpreter finalizati…

76bcc46

…on (pythongh-128640)

Incidentally, this also fixed the warning not showing up if a subinterpreter wasn't
cleaned up via _interpreters.destroy. I had to update some of the tests as a result.
(cherry picked from commit 9859791)

Co-authored-by: Peter Bierma <[email protected]>

bedevere-app Bot commented May 19, 2025

Copy link

Copy Markdown

GH-134254 is a backport of this pull request to the 3.14 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label

May 19, 2025

bedevere-bot commented May 19, 2025

Copy link

Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot iOS ARM64 Simulator 3.x (tier-3) has failed when building commit 9859791.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1380/builds/3740) and take a look at the build logs.
Check if the failure is related to this commit (9859791) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1380/builds/3740

Summary of the results of the build (if available):

Click to see traceback logs

remote: Enumerating objects: 32, done.        
remote: Counting objects:   3% (1/32)        
remote: Counting objects:   6% (2/32)        
remote: Counting objects:   9% (3/32)        
remote: Counting objects:  12% (4/32)        
remote: Counting objects:  15% (5/32)        
remote: Counting objects:  18% (6/32)        
remote: Counting objects:  21% (7/32)        
remote: Counting objects:  25% (8/32)        
remote: Counting objects:  28% (9/32)        
remote: Counting objects:  31% (10/32)        
remote: Counting objects:  34% (11/32)        
remote: Counting objects:  37% (12/32)        
remote: Counting objects:  40% (13/32)        
remote: Counting objects:  43% (14/32)        
remote: Counting objects:  46% (15/32)        
remote: Counting objects:  50% (16/32)        
remote: Counting objects:  53% (17/32)        
remote: Counting objects:  56% (18/32)        
remote: Counting objects:  59% (19/32)        
remote: Counting objects:  62% (20/32)        
remote: Counting objects:  65% (21/32)        
remote: Counting objects:  68% (22/32)        
remote: Counting objects:  71% (23/32)        
remote: Counting objects:  75% (24/32)        
remote: Counting objects:  78% (25/32)        
remote: Counting objects:  81% (26/32)        
remote: Counting objects:  84% (27/32)        
remote: Counting objects:  87% (28/32)        
remote: Counting objects:  90% (29/32)        
remote: Counting objects:  93% (30/32)        
remote: Counting objects:  96% (31/32)        
remote: Counting objects: 100% (32/32)        
remote: Counting objects: 100% (32/32), done.        
remote: Compressing objects:   6% (1/16)        
remote: Compressing objects:  12% (2/16)        
remote: Compressing objects:  18% (3/16)        
remote: Compressing objects:  25% (4/16)        
remote: Compressing objects:  31% (5/16)        
remote: Compressing objects:  37% (6/16)        
remote: Compressing objects:  43% (7/16)        
remote: Compressing objects:  50% (8/16)        
remote: Compressing objects:  56% (9/16)        
remote: Compressing objects:  62% (10/16)        
remote: Compressing objects:  68% (11/16)        
remote: Compressing objects:  75% (12/16)        
remote: Compressing objects:  81% (13/16)        
remote: Compressing objects:  87% (14/16)        
remote: Compressing objects:  93% (15/16)        
remote: Compressing objects: 100% (16/16)        
remote: Compressing objects: 100% (16/16), done.        
remote: Total 17 (delta 15), reused 2 (delta 1), pack-reused 0 (from 0)        
From https://github.com/python/cpython
 * branch                    main       -> FETCH_HEAD
Note: switching to '9859791f9e116c827468f307ac0770286c975c8b'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 9859791f9e1 gh-128639: Don't assume one thread in subinterpreter finalization (gh-128640)
Switched to and reset branch 'main'

configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

make: *** [testios] Error 65

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk mentioned this pull request

May 19, 2025

[3.14] gh-128639: Don't assume one thread in subinterpreter finalization (gh-128640) #134254

Closed

ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull request

May 19, 2025


          Revert "pythongh-128639: Don't assume one thread in subinterpreter fi…

79ed70e

…nalization (pythongh-128640)"

This reverts commit 9859791.

ericsnowcurrently pushed a commit that referenced this pull request

May 19, 2025


          Revert "gh-128639: Don't assume one thread in subinterpreter finaliza…

27bd082

…tion (gh-128640)" (gh-134256)

This reverts commit 9859791.

The original change broke the iOS and android buildbots, where the tests are run single-process.

ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull request

May 24, 2025


          Reapply "pythongh-128639: Don't assume one thread in subinterpreter f…

b14aa73

…inalization (pythongh-128640)" (pythongh-134256)

This reverts commit 27bd082.

ZeroIntensity mentioned this pull request

May 24, 2025

gh-128639: Don't assume one thread in subinterpreter finalization with fixed daemon thread support #134606

Merged

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request

Jul 12, 2025


          pythongh-128639: Don't assume one thread in subinterpreter finalizati…

29d735a

…on (pythongh-128640)

Incidentally, this also fixed the warning not showing up if a subinterpreter wasn't
cleaned up via _interpreters.destroy. I had to update some of the tests as a result.

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request

Jul 12, 2025


          Revert "pythongh-128639: Don't assume one thread in subinterpreter fi…

521fb74

…nalization (pythongh-128640)" (pythongh-134256)

This reverts commit 9859791.

The original change broke the iOS and android buildbots, where the tests are run single-process.

taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request

Aug 4, 2025


          pythongh-128639: Don't assume one thread in subinterpreter finalizati…

c546d00

…on (pythongh-128640)

Incidentally, this also fixed the warning not showing up if a subinterpreter wasn't
cleaned up via _interpreters.destroy. I had to update some of the tests as a result.

taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request

Aug 4, 2025


          Revert "pythongh-128639: Don't assume one thread in subinterpreter fi…

c92b809

…nalization (pythongh-128640)" (pythongh-134256)

This reverts commit 9859791.

The original change broke the iOS and android buildbots, where the tests are run single-process.

ZeroIntensity added a commit that referenced this pull request

Sep 17, 2025


          gh-128639: Don't assume one thread in subinterpreter finalization wit…

a648813

…h fixed daemon thread support (GH-134606)

This reapplies GH-128640.

miss-islington mentioned this pull request

Sep 17, 2025

[3.14] gh-128639: Don't assume one thread in subinterpreter finalization with fixed daemon thread support (GH-134606) #139050

Merged

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 17, 2025


          pythongh-128639: Don't assume one thread in subinterpreter finalizati…

7b8c082

…on with fixed daemon thread support (pythonGH-134606)

This reapplies pythonGH-128640.
(cherry picked from commit a648813)

Co-authored-by: Peter Bierma <[email protected]>

ZeroIntensity added a commit that referenced this pull request

Oct 7, 2025


          [3.14] gh-128639: Don't assume one thread in subinterpreter finalizat…

cec4ddf

…ion with fixed daemon thread support (GH-134606) (GH-139050)

gh-128639: Don't assume one thread in subinterpreter finalization with fixed daemon thread support (GH-134606)

This reapplies GH-128640.
(cherry picked from commit a648813)

Co-authored-by: Peter Bierma <[email protected]>

Reviewers

sergey-miryanov

sergey-miryanov left review comments

ericsnowcurrently

ericsnowcurrently approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-subinterpreters

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

ZeroIntensity commented Jan 8, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity Jan 8, 2025

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently May 19, 2025

Choose a reason for hiding this comment

Uh oh!

vstinner commented Feb 25, 2025

Uh oh!

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently May 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-app Bot commented May 19, 2025

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented May 19, 2025

Uh oh!

hugovk commented May 19, 2025

Uh oh!

miss-islington-app Bot commented May 19, 2025

Uh oh!

bedevere-app Bot commented May 19, 2025

Uh oh!

bedevere-bot commented May 19, 2025

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ZeroIntensity commented Jan 8, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.