robotools / vanilla

A Pythonic wrapper around Cocoa.
MIT License
78 stars 28 forks source link

VanillaCallbackWrapper might cause a leak #148

Closed schriftgestalt closed 3 years ago

schriftgestalt commented 3 years ago

It seems that if I assign a method of a class that is a subclass of NSObject to a callback of a EditText than VanillaCallbackWrapper is storing a strong reference to that object (in initWithCallback_). Using self.callback = weakref.ref(callback) fixes it for me. I’m not sure if that would cause any trouble elsewhere.

I’m not sure if the python garbage collector is clever enough to find that loop (object retains the text field and the text fields VanillaCallbackWrapper is retaining the object) and break it so this might leak in pure python, too.

justvanrossum commented 3 years ago

Can you post a complete example? You can't create a weakref to a method of an NSObject subclass, so I don't understand your suggested fix.

Creating a weakref to a Python bound method is problematic, as it will go away immediately if you don't keep a hard reference.

In vanilla, cycles like this usually get cleaned up when the window closes, and it seems that should be done in this case, too:

https://github.com/robotools/vanilla/blob/04af4a5c1627ca810dd46b83d92cc354a4ddcbf8/Lib/vanilla/vanillaBase.py#L69-L71

schriftgestalt commented 3 years ago

Thanks for you comments.

When you can add objc.python_method to the method to prevent the conversion into a selector. That is needed a lot in python 3 as it is more strict and will fail when the method name is not a valid signature (number of underscores != number of arguments).

In this occasion, the window part is not used because we only use the main view as UI inside Glyphs (e.g. in the sidebar). So the window is never shown and never closed and thous will not call _breakCycles.

Shouldn’t a method reference be valid until the object it came from is deallocated? Where else would you get a method reference from?

justvanrossum commented 3 years ago

That's not how method references work in Python. A bound method object keeps a reference to the object and it is not cached. Each time you ask for a bound method you get a new bound method object. This is to avoid cyclic references. So yes, the lifetime of the bound method object can be shorter than the lifetime of the underlying object.

class Example:
    def method(self):
        pass

e = Example()
print(e.method == e.method)  # sure, they're equivalent/equal
print(e.method is e.method)  # but they are distinct objects

_breakCycles() is an essential piece of vanilla machinery. If you use vanilla objects in non-vanilla contexts, it is your responsibility to call it at the appropriate time.

schriftgestalt commented 3 years ago

This is very good to know. Thanks.