pyapp-kit / psygnal

Python observer pattern (callback/event system). Modeled after Qt Signals & Slots (but independent of Qt)
https://psygnal.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
86 stars 15 forks source link

Psygnal prevents deletion of emitted object #300

Closed aeisenbarth closed 8 months ago

aeisenbarth commented 8 months ago

Description

I use psygnal on a metaclass level to be notified when a class instance is created or garbage-collected. This allows to register additional setup/cleanup tasks when a third-party class does not provide an API for that. I had implemented this properly using weak references, so that my implementation itself did not keep hard references.

Since psygnal 0.10.1, it seems the signal keeps a reference to the last emitted argument, preventing its garbage-collection.

What I Did

Minimal example:

def test_emit_should_not_prevent_gc():
    from psygnal import Signal
    from weakref import WeakSet

    class Obj:
        pass

    class SomethingWithSignal:
        changed = Signal(Obj)

    object_instances = WeakSet()
    something = SomethingWithSignal()
    obj = Obj()
    object_instances.add(obj)
    something.changed.emit(obj)
    del obj
    assert len(object_instances) == 0, "Fails if the deleted object is the last emitted one"
    something.changed.emit(Obj())
    assert len(object_instances) == 0, "Passes because another object is the last emitted one"

Example a bit closer to my use case (but without metaclasses etc.)

def test_observing_instantiation_events():
    from psygnal import Signal
    from weakref import WeakSet

    class Obj:
        instances = WeakSet()
        instance_added = Signal(object)
        instance_removed = Signal(object)

        def __init__(self):
            self.instance_added.emit(self)
            self.instances.add(self)

        def __del__(self):
            # Only called when the last reference is deleted and the reference count becomes 0.
            self.instance_removed.emit(self)

    instance = Obj()
    del instance
    assert len(Obj.instances) == 0
tlambert03 commented 8 months ago

Thanks @aeisenbarth for the very clear issue and tests. I’ll take a look later today and release a fix asap

tlambert03 commented 8 months ago

Actually I know exactly what cause it… sorry about that, will fix very soon

tlambert03 commented 8 months ago

v0.10.2 has the fix and is deploying now

aeisenbarth commented 8 months ago

Thanks! All test pass again!