Skip to content
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

Proof of concept: Weakref signalslots #98

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

luchr
Copy link
Contributor

@luchr luchr commented Dec 29, 2022

(Signal-)slots with weak references

Subtitle: Tired of slot-bookkeeping?

Facts

  1. Currently all signals (i.e. instances of the class _pyTTkSignal_obj) are saved inside a list in the class variable _pyTTkSignal_obj._signals
  2. Many widgets create signals, e.g. TTkWindow and its buttons (close, minimize, etc.)
  3. Every signal manages a list of connected slots/listeners in the variable _connected_slots

Consequence

Even closed windows cannot be garbage collected because via the signal list, and the connected slots all the buttons are "reachable" and the buttons have links to their window and hence the windows are reachable.

Solutions

There are many many ways to solve this problem. In the PR I tried one (in my opinion a lazy and elegant one).

There is a WeakrefSlot class. Instances of this class behave (transparently) like "slots", i.e. they are callable. The "only" difference to a "normal" slot is, that the target-function/listener is saved via a weakref.ref or weakref.WeakMethod. Hence the WeakrefSlot instances do not prevent the garbage collection.

Dead WeakrefSlot instances (i.e. WeakrefSlot where the target-function is not reachable anymore because it was garbage collected) are cleaned up lazily and automatically in the emit method.

I've attached a nice test tests/test.ui.024.weakslots.py:
weakslots.webm

Some questions remain

  1. Is it necessary to save all signals in a global list? [If it's needed for pyodine, then perhaps its better to only save it if pyodine is used]
  2. WeakrefSlot can only be used if the target-function/slot is "weakly referable", i.e. it allows for a __weakref__ attribute. Does TermTk.TTkWidget and subclasses want to support this? (see Attention)

Attention

The tests/test.ui.024.weakslots.py code uses a current "bug" in TermTk. All the Widgets have a __dict__ (as discussed in an other PR). That's the reason why e.g. ShowEventWindow.show_event or TTkWindow.close are "weakly referable". Without __weakref__ attribute there is no chance to get a weakref for such objects.

Without a __weakref__ variable for each instance, classes defining __slots__ do not support weak references to its instances. If weak reference support is needed, then add __weakref__ to the sequence of strings in the __slots__ declaration.
https://docs.python.org/3/reference/datamodel.html#notes-on-using-slots

@ceccopierangiolieugenio
Copy link
Owner

Yes, I think I saved everything in a global var in order to clean all the signals in pyodide,
but mainly because I was not aware about the weakref, I need to spend some time to figure out how it properly works.
About the signals/slots by default all of them should be weak and from time to time I was thinking about some complex cleaning routine to dereference everything but the weakref seems to be the correct approach

@@ -88,7 +154,7 @@ def __init__(self, *args, **kwargs):
self._connected_slots = []
_pyTTkSignal_obj._signals.append(self)

def connect(self, slot):
def connect(self, slot, use_weak_ref=False):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to think about having the "weak ref" true by default,
I am not sure what it is going to happen if connected to lambda or nedted functions.
For the naming convention I tried to use the camel case although not recommended by PEP8
because I started coding it following the QT api structure

@luchr
Copy link
Contributor Author

luchr commented Jan 1, 2023

About the signals/slots by default all of them should be weak and from time to time I was thinking about some complex cleaning routine to dereference everything but the weakref seems to be the correct approach

If all signals should be weak, then all persons (including the users of pyTermTk) must be very careful:

  1. I already give some warnings in the Proof of Concept. An inline lambda function has only one reference; the one handed in to connect. If connect only uses a weakref to this lambda function then immediately after connect has finished, the only reference went out of scope and the lambda-inline function is garbage collected immediately. I call this a "dead on arrival" reference.
  2. Not every object is "weakly referable". A necessary condition is that such an instance must allow for an __weakref__ attribute (either by explicitly naming it in __slots__ or by (implicitly) allowing this because the instance has a __dict__ where many attributes can be saved).

I don't have a strong opinion, but I slightly prefer the option to give the caller of the connect method the choice ... [whatever the default for use_weak_ref or useWeakRef is].

This is only a Proof of Concept PR. It is not intended to be merged but to show the pros and cons. And please do not merge because it heavily relies on the fact that (at the moment) many widgets (like a TTkWindow) have __dict__ (which is not on purpose and may change in the near future). If this changes because every parent of TTkWindow will use __slots__ then somewhere the __weakref__ slot must be mentioned in order to be able to use weakrefs for bound methods like TTkWindow.close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants