Qt GUI leaking references: closed main windows will not get garbage collected (memory leak, gc, closed wallet)

[SomberNight]

On https://github.com/spesmilo/electrum/commit/0294844c11a4da2b30faba26d4649c2aae24860f. (using Qt GUI)

1. open wallet1
2. open wallet2
3. close wallet2
4. repeat steps (2) and (3) a couple times

The closed windows are not getting garbage collected. There seem to be references to them left somewhere.

Try in console:

import threading; from electrum.util import DebugMem; from electrum.gui.qt import ElectrumWindow; d = DebugMem([ElectrumWindow], 1)
threading.Thread(target=d.run, args=()).start()

[SomberNight]

One (of potentially many...) reason is pyqt signals...

https://stackoverflow.com/questions/47941743/lifetime-of-object-in-lambda-connected-to-pyqtsignal/47945948#47945948

Note that when it comes to signal/slot connections, PyQt treats wrapped C++ slots and Python instance methods differently. The reference counts of these types of callable are not increased when they are connected to signals, whereas lambdas, defined functions, partial objects and static methods are. This means that if all other references to the latter types of callable are deleted, any remaining signal connections will keep them alive. Disconnecting the signals will allow such connected callables to be garbage-collected, if necessary.

[ecdsa]

maybe related: https://github.com/spesmilo/electrum/blob/master/electrum/gui/qt/main_window.py#L207

[ecdsa]

note: it seems that new_fx_quotes_signal and new_fx_history_signal are never disconnected

[SomberNight]

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.

[cculianu]

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.

[cculianu]

PSA: In Qt closed windows don't usually die. A closed window is not a deleted window. You can always call show() on a closed-window in Qt.

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:

        widow.setAttribute(Qt.WA_DeleteOnClose)

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 widget.deleteLater() or using the above WA_DeleteOnClose), the Python side's __del__ gets called pretty soon after the window is closed or the deleteLater() is issued.

[SomberNight]

Interesting. Thanks.

FWIW in my code in Electron Cash I always do this: widow.setAttribute(Qt.WA_DeleteOnClose)

I cannot find this in the codebase. Where is this done?

[cculianu]

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.

https://github.com/clifordsymack/Electron-Cash/

[cculianu]

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.

[cculianu]

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.

[cculianu]

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: https://github.com/spesmilo/electrum/blob/5469e3668ef2d7405849c8e6e88f67a4acfc399b/electrum/gui/qt/exception_window.py#L140

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.

[cculianu]

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: https://github.com/Electron-Cash/Electron-Cash/commit/6a3d76b0ab7bf3fe58390100f5bf2ab8a3261d87

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.

[SomberNight]

note: useful for debugging:

import gc, weakref
w = weakref.ref(window.gui_object.windows[1])
gc.collect()
gc.get_referrers(w())

[SomberNight]

Now with https://github.com/spesmilo/electrum/commit/2105c6c4e6085f9c53eb58dfe8447f2b5b1abad7, https://github.com/spesmilo/electrum/commit/0f6cbfba8ec96c886582de270e106d7cae0fb322, https://github.com/spesmilo/electrum/commit/0ee73378c98f8f209fb5ae32763e8771522fc703, main windows and wallets properly get garbage collected.

The really crucial bit that really fixed it I think is the Exception_Hook not keeping a strong reference to the main_window

Thanks for this hint, it was useful!