GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-36479: Exit threads when interpreter is finalizing rather than runtime by nanjekyejoannah · Pull Request #12679 · python/cpython

Conversation

@nanjekyejoannah

@nanjekyejoannah nanjekyejoannah commented Apr 3, 2019 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

I have added changes to exit threads when the interpreter is finalizing.

I have halted to update this until PR until GH-12667 is merged.

https://bugs.python.org/issue36479

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 3, 2019
@bedevere-bot bedevere-bot added the awaiting review label Apr 3, 2019
@nanjekyejoannah

nanjekyejoannah commented Apr 3, 2019

Copy link
Copy Markdown
Contributor Author

cc @ericsnowcurrently , @ncoghlan
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nanjekyejoannah nanjekyejoannah changed the title bpo-36479: Exit threads when interpreter is finalizing rather than runtime [WIP]bpo-36479: Exit threads when interpreter is finalizing rather than runtime Apr 3, 2019
ericsnowcurrently
ericsnowcurrently reviewed Apr 3, 2019
View reviewed changes
Comment thread Python/ceval.c Outdated

@ericsnowcurrently ericsnowcurrently Apr 3, 2019

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.

Please check _Py_CURRENTLY_FINALIZING(). I suspect that it doesn't do the right thing. It may only matter for the main interpreter (e.g. during runtime finalization).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nanjekyejoannah
nanjekyejoannah force-pushed the issue36479 branch 2 times, most recently from a132f03 to 68bc067 Compare April 25, 2019 17:19
@nanjekyejoannah nanjekyejoannah changed the title [WIP]bpo-36479: Exit threads when interpreter is finalizing rather than runtime bpo-36479: Exit threads when interpreter is finalizing rather than runtime [Witing on merge of GH-12667] Apr 25, 2019
@nanjekyejoannah
nanjekyejoannah force-pushed the issue36479 branch 3 times, most recently from 607a345 to b8a1690 Compare April 25, 2019 22:25
@nanjekyejoannah nanjekyejoannah changed the title bpo-36479: Exit threads when interpreter is finalizing rather than runtime [Witing on merge of GH-12667] bpo-36479: Exit threads when interpreter is finalizing rather than runtime [Waiting on merge of GH-12667] Apr 25, 2019
ncoghlan
ncoghlan reviewed Apr 26, 2019
View reviewed changes
Comment thread Doc/c-api/init.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@nanjekyejoannah
Exit threads when interpreter is finalizing rather than runtime.
a19bbd4
@nanjekyejoannah
nanjekyejoannah force-pushed the issue36479 branch from 5f145b3 to a19bbd4 Compare May 5, 2019 16:16
@blurb-it
📜🤖 Added by blurb_it.
515e8bb
@nanjekyejoannah nanjekyejoannah changed the title bpo-36479: Exit threads when interpreter is finalizing rather than runtime [Waiting on merge of GH-12667] bpo-36479: Exit threads when interpreter is finalizing rather than runtime May 5, 2019
vstinner
vstinner requested changes May 6, 2019
View reviewed changes
Comment thread Python/ceval.c
PyInterpreterState *interp = tstate->interp;

/* _Py_Finalizing is protected by the GIL */
if (_Py_IsFinalizing() && !_Py_CURRENTLY_FINALIZING(tstate)) {

@vstinner vstinner May 6, 2019

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.

With this change, _Py_CURRENTLY_FINALIZING() becomes even more weird that it was: you rely on a global and unique PyRuntimeState to check which thread is currently being finalized. I don't think that it's correct. It should be possible to finalize 2 interpreters in parallel which both have their own thread.

For me, it would make more sense to move "PyThreadState *finalizing;" from _PyRuntimeState to PyInterpreterState and remove PyInterpreterState.finalizing.

What's your call Eric? @ericsnowcurrently

Sorry, something went wrong.

Uh oh!

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

@ncoghlan ncoghlan May 8, 2019

Copy link
Copy Markdown
Contributor

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.

The thread state in finalizing actually means "This is the thread that is finalizing the runtime", so other threads can easily tell that another thread is responsible for cleaning things up.

Sorry, something went wrong.

Uh oh!

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

@ncoghlan ncoghlan May 8, 2019

Copy link
Copy Markdown
Contributor

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.

However @pablogsal pointed out a more serious problem here, which is that simply moving the existing check will result in:

  • potentially causing threads belonging to other subinterpreters to exit
  • potentially causing threads belonging to the embedding application to exit

The latter problem already exists, but moving the cleanup to EndInterpreter makes it easier to hit.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented May 6, 2019

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-bot bedevere-bot added awaiting changes and removed awaiting review labels May 6, 2019
ncoghlan
ncoghlan requested changes May 8, 2019
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

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.

Blocking merge as per https://bugs.python.org/issue36479#msg341882 (we need to actively constrain the cleanup to daemon threads created by the interpreter being cleaned up, and simply report an error to other threads)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented May 10, 2019

Copy link
Copy Markdown
Member

The thread state in finalizing actually means "This is the thread that is finalizing the runtime", so other threads can easily tell that another thread is responsible for cleaning things up.

I don't understand what is "runtime" here. _PyRuntimeState doesn't contain threads: PyInterpreterState does. IMHO the field should be moved from _PyRuntimeState to PyInterpreterState.

Right now, Py_EndInterpreter() doesn't call Py_FinalizeEx(): but that's wrong. To really have isolated interpreter, each interpreter should clear everything it owns. For example, each interpreter should join all threads that it spawned.
All reactions

Sorry, something went wrong.

Uh oh!

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

ncoghlan commented May 10, 2019

Copy link
Copy Markdown
Contributor

@vstinner PyFinalize_Ex kills the entire runtime, so we can't call it just because a subinterpreter went away. Instead, we have to migrate cleaning up particular resources from PyFinalize_Ex to Py_EndInterpreter as this PR does.

However, I also agree that for this PR to work there needs to be a new field in the individual interpreter state that tracks which thread is cleaning up that particular interpreter.

And then once we have that per-interpreter field and are using it consistently, then we can ask if we still need a separate runtime level "finalizing" marker, or if the finalizing marker for the main interpreter will be sufficient (which will mean addressing the fact that Py_EndInterpreter never actually gets called on the main interpreter - Py_FinalizeEx has its own copy of the pieces it wants to use, and does a bunch of other more destructive things while keeping at least one thread state alive until the last possible moment).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella csabella added the DO-NOT-MERGE label Jun 1, 2019
@nanjekyejoannah

nanjekyejoannah commented Jun 24, 2019

Copy link
Copy Markdown
Contributor Author

Closing this for now. I may revisit in the future in a new PR. Anyone else can look at this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nanjekyejoannah nanjekyejoannah closed this Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently left review comments
@vstinner vstinner vstinner requested changes
@ncoghlan ncoghlan ncoghlan requested changes

Uh oh!

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

Assignees

No one assigned

Labels

awaiting changes DO-NOT-MERGE

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.

7 participants

@nanjekyejoannah @bedevere-bot @vstinner @ncoghlan @ericsnowcurrently @the-knights-who-say-ni @csabella
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 01402520 — 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.