Skip to content

Ensure the gc module is available during interpreter exit #4166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 27, 2015

Conversation

embray
Copy link
Contributor

@embray embray commented Feb 25, 2015

This is directly related to #4165. Another side effect of run_setup is that any modules that are imported during the run_setup call are later removed from sys.modules. If there are no other references to that module they will be garbage collected and any previous references to globals in that module will be replaced with None.

Because destroy_all is registered as an atexit function it will run even after all the module itself has been garbage collected (there are other contexts where this might happen as well). This seems to otherwise work since it was changed to a classmethod, so now the Gcf class itself is still hanging around. But it may be necessary to re-import the gc module if it's to be made use of.

@@ -84,6 +84,7 @@ def destroy_fig(cls, fig):

@classmethod
def destroy_all(cls):
import gc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, this is only in this method since it also is registered as an atexit function. Otherwise it wouldn't be necessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose it would also be worth adding a comment that this is here deliberately (since gc is already imported at the module level any delinter is going to complain about this...)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good point.

Copy link
Member

Choose a reason for hiding this comment

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

the # noqa flag works on most linters?

@mdboom
Copy link
Member

mdboom commented Feb 25, 2015

Seems fine -- and pretty harmless in the 99.9% of non-corner cases where this is a non-issue.

@tacaswell tacaswell added this to the next point release milestone Feb 25, 2015
@tacaswell
Copy link
Member

👍 from me modulo a comment / linter protection flag.

@tacaswell tacaswell merged commit ea367e2 into matplotlib:master Feb 27, 2015
@tacaswell
Copy link
Member

Merged locally to add comment + linter flage

@embray embray deleted the patch-2 branch February 27, 2015 16:30
@embray
Copy link
Contributor Author

embray commented Feb 27, 2015

@tacaswell Where did you merge locally? GitHub just shows that you merged my PR as-is.

@embray
Copy link
Contributor Author

embray commented Feb 27, 2015

Or to be more specific, I'm wondering how you got GitHub to recognize this as merged, while also including your own changes. It would be lovely if I knew how to do that.

@tacaswell
Copy link
Member

Merged as 13a8f31

GH keeps track of 'mergestate' based on if the commits on the merge-branch are accessible from the target branch so all you have to do is merge locally and push to the target branch and GH is happy.

@stonebig stonebig mentioned this pull request Sep 2, 2015
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants