connect()'s refcounting behavior seems buggy, and should be documented

[Ben Rudiak-Gould]

On 19/03/2022 02:28, Kovid Goyal wrote:
> On Fri, Mar 18, 2022 at 05:26:13PM +0000, Phil Thompson wrote:
>> I've changed the PyQt6 behaviour to keep a reference to bound methods.
>
> If its a strong reference it will create reference cycles.
> Something as simple as:
>
> class Widget(QDialog):
>
>     def __init__(self):
>         QDialog.__init__(self)
>         self.bb = QDialogButtonBox(self)
>         self.bb.accepted.connect(self.accept)
>
>     def accept(self):
>         ...
>
> is now a reference cycle that the python garbage collector cannot
> collect.

When a bound method is passed to connect(), PySide6 doesn't hold a
reference to the bound method at all: It extracts its two fields, the
"self" object and the function, and holds a weak reference to "self"
(some instance of Widget in your example) and a strong reference to
the function (Widget.accept).

Assuming the PyQt6 change makes it compatible with PySide6, the only
difference between the old and new behavior will be that the reference
count of Widget.accept will be incremented for the lifetime of the
connection. The reference count of the Widget instance won't be
incremented. Functions defined in classes usually stick around for the
lifetime of the program anyway. The only way this change would be
observable is if you deleted/replaced Widget.accept, and it had a
default argument value or annotation or something that was a large
object that now won't be collected until the connection goes away.


On Sat, Mar 19, 2022 at 3:44 AM Yuya Nishihara <yuya at tcha.org> wrote:
> class Widget(QDialog):
>     def __init__(self, parent=None):
>         [...]
>         self.bb.accepted.connect(lambda: self.accept()) # strong ref
>         self.t = QTimer()
>         self.t.timeout.connect(self.onTimeout)
>         self.t.start(100)
>     [...]
>
> def kick():
>     w = Widget()
>     w.exec()
>     print('w should ideally be deleted here')
>
> [...]

If I understand your example correctly, and PT made the change I think
he did, it won't affect this example. The only difference between the
new and old behavior is that the reference count of Widget.onTimeout
will be incremented. connect(lambda: self.accept()) has always held a
strong reference to self, since PyQt4/PySide1. (I asked for that to be
documented.)