gh-128639: Don't assume one thread in subinterpreter finalization by ZeroIntensity · Pull Request #128640 · python/cpython

ZeroIntensity · 2025-01-08T17:51:47Z

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

ZeroIntensity · 2025-01-08T19:40:28Z

Programs/_testembed.c

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


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.

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

vstinner · 2025-02-25T15:34:27Z

cc @ericsnowcurrently

Python/pylifecycle.c

…rpreter-thread-shutdown

ericsnowcurrently

mostly LGTM

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

Python/pylifecycle.c

ericsnowcurrently · 2025-05-19T14:57:05Z

Programs/_testembed.c

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


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

Lib/test/test_threading.py

Lib/test/test_interpreters/test_lifecycle.py

Lib/test/test_interpreters/test_api.py

bedevere-app · 2025-05-19T15:15:03Z

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.

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

ericsnowcurrently

LGTM

Thanks for doing this!

ericsnowcurrently · 2025-05-19T16:24:37Z

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

hugovk · 2025-05-19T16:32:44Z

Yep, crash bugfixes can be backported :)

miss-islington-app · 2025-05-19T16:57:22Z

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

…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 <zintensitydev@gmail.com>

bedevere-app · 2025-05-19T16:57:34Z

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

bedevere-bot · 2025-05-19T17:13:19Z

⚠️⚠️⚠️ 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

…nalization (pythongh-128640)" This reverts commit 9859791.

…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.

…inalization (pythongh-128640)" (pythongh-134256) This reverts commit 27bd082.

…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.

…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.

…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.

…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.

…h fixed daemon thread support (GH-134606) This reapplies GH-128640.

…on with fixed daemon thread support (pythonGH-134606) This reapplies pythonGH-128640. (cherry picked from commit a648813) Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

…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 <zintensitydev@gmail.com>

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

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

sergey-miryanov reviewed Mar 15, 2025

View reviewed changes

Python/pylifecycle.c Show resolved Hide resolved

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

bedevere-app bot removed the awaiting review label May 19, 2025

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

1555731

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

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

bedevere-app bot added awaiting merge and removed awaiting changes labels May 19, 2025

ericsnowcurrently merged commit 9859791 into python:main May 19, 2025
45 of 46 checks passed

bedevere-app bot removed the awaiting merge label May 19, 2025

ZeroIntensity deleted the subinterpreter-thread-shutdown branch May 19, 2025 16:24

ZeroIntensity added the needs backport to 3.14 bugs and security fixes label May 19, 2025

bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label May 19, 2025

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.

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

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

Uh oh!

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!

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

ZeroIntensity commented Jan 8, 2025 •

edited by bedevere-app bot

Loading