-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Prevent GC of animations and widgets still in use. #16221
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
Conversation
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 am 👎 on this.
If animation / widgets objects can keep themselves alive and the user loses their references to them, I don't see how they can recover one to stop / disconnect it.
If we are going to go this route of keeping animations alive, I think monkey patching them onto the Figure
is a better option than hiding hard references in lambdas in the callbacks.
If they want to disconnect it they just need to keep a ref to it from the start. I see this as similar to starting a thread in Python without keeping a ref to it: from threading import Thread; from time import sleep
def work():
while True:
sleep(1); print("still here")
Thread(target=work).start() if you haven't kept a ref on the thread you can't e.g. join() or do anything with it, and it doesn't get garbage collected. (And this is pretty close to the issue at hand, as we're basically arranging for stuff to be called in a separate thread.) So there's definitely prior art for it.
Well, I really don't like the fact that we use weakrefs anyways. This is also inconsistent with how all GUI toolkits work -- they also keep hardrefs to everything, that's (I think) how the animations still work here (the GUI timers keep hard refs to them). |
Keeping an internal reference via a lambda is quite arcane. If we really want to use that, it should be documented more extensively. However, I have the feeling that this is not the right design. In particular, (if I understand this magic correctly) animations would have a zombie life and we'd burden the user to prevent that if they don't want it. I don't want to re-iterate #1656, but some ideas might clarify the nature of the issue: Why does the figure not hold a reference to the animation? In contrast to artists, which are part of the figure, an animation is a kind of FigureController (name made up). It sits outside the figure, manipulates its artists from time to time and calls for redraws. In that sense it's quite similar to GUI toolkits. The controller holds the figure, not the other way round. In contrast to GUIs, there is nobody naturally holding a reference to the FigureController in case of animations. And then it gets garbage collected. Why it might nevertheless be a good idea for the figure to hold a reference The analogy with threads is not too far fetched. However, given that our average user is not necessarily an experienced programmer, I don't expect him to know about references, garbage collection or analogies with threads. IMHO it would be ok if any FigureController registers itself with the figure it wants to control. That can be as simple as the figure holding a private list of registered controllers. The figure doesn't have to know what the controller is and doesn't do anything with it (could be changed if there was a use later). This is still a rather loose coupling. From the reference point of view, if nobody outside holds a reference to the figure anymore, figure and animation will get cleaned up together (whereas a zombie animation would keep the figure alive). Yes, two PRs towards #1656 adding animation references to the figure were rejected, but I still find it the best solution. Maybe the rephrasing as FigureController gives a new logic to motivate that. The proposed solution proposed here works for both animations and widgets. |
(I allowed myself to reorder your paragraphs to present the reply better.)
Well, it's needed only because CallbackRegistry has the (questionable, IMO) semantics of only keeping weakrefs to bound methods -- which means that
The inexperienced programmer is actually exactly the one that would benefit the most here. Right now if you look at the animation examples they all finish with the assignment of an unused variable (
Actually we already behave in the opposite way compared to GUI toolkits: in PyQt if you create a QMainWindow() but don't assign it to anything, then it will be gc'd, whereas we maintain an explicit cache of
(I'm fine with that too. But it's really a lot of machinery for something that should be simple: "if something is supposed to handle callbacks, don't let it be dropped -- or, if you really want it to be weakref'd, set up the weakref yourself at connection time".) |
We shouldn't build functionality on accidental and undesired subtle code behavior.
Fair enough. But nevertheless, my point holds that the best approach here would be for the figure to hold a strong reference to the animation. |
My point is that this brittleness/subtleness can, AFAICT, only be removed by making CallbackRegistry hold strong references. |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
PR Summary
Closes #1656; improves the situation of #3105 (IMO).
It's really a mostly policy choice at that point.
PR Checklist