-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Qt GUI leaking references: closed main windows will not get garbage collected (memory leak, gc, closed wallet) #4905
Comments
One (of potentially many...) reason is pyqt signals...
|
note: it seems that |
Hardly any of the pyqt signals are disconnected... though going from the above linked paragraph I think if they are connected to instance methods they don't need to be. |
Hmm. If you think about it, it shouldn't matter if it's connected to a lambda, an inner function, or a member. Garbage collection should detect such cycles anyway and deal with them intelligently as long as the rest of the program can't get to the reference/reference graphs in question. I did notice this as well. Windows don't ever die. You could potentially call self.deleteLater() after a window is closed and Qt will explicitly delete the C++ object. I am not sure if that will cause the python side to blow up or if it will work and actually help the python side decide to actually kill the Python objects (it may just work!). This is definitely an issue though for Electron Cash as well and it's bugged me for some time now. So commenting here to get emailed about how this develops. |
PSA: In Qt closed windows don't usually die. A closed window is not a deleted window. You can always call This is likely why the Python side's object is being kept alive -- likely by sip or by PyQt. FWIW in my code in Electron Cash I always do this:
For windows that need to be deleted on close... and the windows get deleted on close! Perhaps C++ is keeping the Python references "alive". Thus, you may need to tell the Qt-side to delete them -- in which case the Python side should del them as well. In my tests after a C++ delete of a widget (either by calling |
Interesting. Thanks.
I cannot find this in the codebase. Where is this done? |
Oh.. I lied. I do deleteLater(). I was doing that up until I realized I wanted more control over when some of the popups would die. So I switched over to deleteLater().. And we're working on this new fork that will be merged to master soon. |
I did some more investigation -- they sometimes get gc'd when a duplicate window appears (for the same wallet), at least on EC. Not sure what the deal is here. I suspect that weakrefs should be used rather than normal strong references for a lot of the internal signal/slot connections and the lambdas. That may be enough to fix the situation. I looked at what's keeping a reference to the ElectrumWindow and it's a LOT of stuff -- more investigation is needed. One day when I get some free time: I'm going to try in Electron Cash to make as many of the signals/slots as can be (for internal stuff) use weak references and see if that fixes the situation. |
Oh -- and deleteLater for the ElectrumWindow will force the Qt side to die but the Python stub still stays around -- and if it's ever accessed it'll throw a PyQt exception. So in this case it definitely is better to rely on the Python GC. deleteLater() works in my code because I'm meticulous about cleanup. But it doesn't work with the ElectrumWindow. I only started thinking about using python weakrefs recently (we have them in Objective-C but I figured they aren't needed in Python so stopped thinking about using them). I always considered the Python GC to be enough but I guess is pretty conservative and can get stumped... so they might be needed. |
I figured it out!!!!! (In Electron Cash, at least -- but I suspect Electrum has the same issue). I had to get rid of a bunch of strong references and do other magicks. This: electrum/electrum/gui/qt/exception_window.py Line 140 in 5469e36
Was the last thing keeping the window from being GC'd. It's keeping a strong reference to the windows. This code is pretty weird in that it keeps stepping on its own toes, reinstalling the exception handler each time for each window -- and has a handle to a window that may or may not still be open. I rewrote this module slightly to be more of a real singleton that doesn't keep references to dead windows. I'm still reviewing my code and working out the kinks but i'll push a commit at some point and hopefully you guys can copy/get inspired by it. |
Sorry to keep spamming you on this issue but I was very motivated to handle this. My C++ sensibilities were offended by the leaks. :) This is done now in EC. See this commit: Electron-Cash@6a3d76b I think most of that commit is just me being fancy. The really crucial bit that really fixed it I think is the Exception_Hook not keeping a strong reference to the main_window -- instead the Exception_Hook was made to not really depend on any main_window instances being alive for it to properly function and is a real singleton now. (If you think about it, the Exception_Hook has a longer lifetime than any 1 main_window, and it isn't clear which main_window is the active one or which wallet generated the exception -- so it keeping a reference to the main_window was a bug, really, even without this GC issue). FWIW: It turns out the pyqt signals are mostly well behaved, surprisingly. They do indeed generate circular references but I'm pretty sure the GC deals ok with that.. in my testing just having lots of signal/slots connecting to lambdas or bound methods was not enough to block gc. The gc does clean up QObjects with a circular reference zoo... eventually. |
related #4905 related Electron-Cash/Electron-Cash@6a3d76b conceptually did not really make sense that the Exception_Hook kept a reference to an ~arbitrary main window (preventing gc)
related: #4905 -- when closing a wallet, it can get gc-ed now TODO: PayServer needs to choose wallet somehow
note: useful for debugging:
|
related spesmilo#4905 related Electron-Cash/Electron-Cash@6a3d76b conceptually did not really make sense that the Exception_Hook kept a reference to an ~arbitrary main window (preventing gc)
related: spesmilo#4905 -- when closing a wallet, it can get gc-ed now TODO: PayServer needs to choose wallet somehow
On 0294844.
(using Qt GUI)
wallet1
wallet2
wallet2
The closed windows are not getting garbage collected.
There seem to be references to them left somewhere.
Try in console:
The text was updated successfully, but these errors were encountered: