gh-128639: Don't assume one thread in subinterpreter finalization with fixed daemon thread support #134606
Conversation
…inalization (pythongh-128640)" (pythongh-134256) This reverts commit 27bd082.
ZeroIntensity
added
topic-subinterpreters
🔨 test-with-buildbots
This comment was marked as outdated.
This comment was marked as outdated.
bedevere-bot
removed
the
🔨 test-with-buildbots
ZeroIntensity
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit a569355 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F134606%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
!buildbot refleak |
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit f85a291 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F134606%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
|
@ericsnowcurrently Yay, buildbots passed on this one! |
There was a problem hiding this comment.
Like with gh-128640, mostly LGTM. I've left a few minor comments.
|
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 |
This comment has been minimized.
This comment has been minimized.
ZeroIntensity
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit 62bd653 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F134606%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
These cause crashes due to an existing bug with subinterpreters.
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @ericsnowcurrently: please review the changes made to this pull request. |
…interp-thread-shutdown
|
Thanks @ZeroIntensity for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…on with fixed daemon thread support (pythonGH-134606) This reapplies pythonGH-128640. (cherry picked from commit a648813) Co-authored-by: Peter Bierma <[email protected]>
|
GH-139050 is a backport of this pull request to the 3.14 branch. |
3 participants
This is a reapplication of #128640, with daemon thread hanging fixed.
The key part that I was missing was this:
My fix predated these lines, so it didn't make it in originally.
The reason this only failed on the iOS buildbots was because they're single-process. Out of random choice, I picked 100 seconds as the sleep time, but since the process would exit before that 100 seconds was over on all the other buildbots, there wasn't any problem. On iOS, the test suite continued to run in the same process after the 100 seconds were up, so the daemon threads would start trying to run again under a deallocated interpreter, causing all sorts of weird crashes.