Automatic unregistering of BindableProperty objects (#4122)

First draft to fix the issue reported in #4109.

Replaces the values in the `binding.bindable_properties` data structure,
which acts as a "registry" for available bindable properties, with
`weakref.finalize` objects. Previously, there was a permanent reference
to the owner of the `BindableProperty`, which was never cleared unless
explicitly removed with `binding.remove`.

I also added some very basic tests for this behavior. You may need to
refactor these slightly.

*What I did not test, and what in theory should still be a problem, is
when 2 models have bindable properties and one model binds to a value of
the other. Then permanent references to the models are kept in
`binding.bindings`, which are never automatically cleaned up.*

---------

Co-authored-by: Andreas Bayer <[email protected]>
Co-authored-by: Falko Schindler <[email protected]>

Author: 3 people

nicegui/binding.py

@@ -3,6 +3,7 @@
 import asyncio
 import dataclasses
 import time
+import weakref
 from collections import defaultdict
 from typing import (
     TYPE_CHECKING,
@@ -32,7 +33,7 @@
 MAX_PROPAGATION_TIME = 0.01
 
 bindings: DefaultDict[Tuple[int, str], List] = defaultdict(list)
-bindable_properties: Dict[Tuple[int, str], Any] = {}
+bindable_properties: Dict[Tuple[int, str], weakref.finalize] = {}
 active_links: List[Tuple[Any, str, Any, str, Callable[[Any], Any]]] = []
 
 T = TypeVar('T', bound=type)
@@ -173,7 +174,8 @@ def __set__(self, owner: Any, value: Any) -> None:
         if has_attr and not value_changed:
             return
         setattr(owner, '___' + self.name, value)
-        bindable_properties[(id(owner), self.name)] = owner
+        key = (id(owner), str(self.name))
+        bindable_properties.setdefault(key, weakref.finalize(owner, lambda: bindable_properties.pop(key, None)))
         _propagate(owner, self.name)
         if value_changed and self._change_handler is not None:
             self._change_handler(owner, value)
@@ -198,9 +200,10 @@ def remove(objects: Iterable[Any]) -> None:
         ]
         if not binding_list:
             del bindings[key]
-    for (obj_id, name), obj in list(bindable_properties.items()):
-        if id(obj) in object_ids:
+    for (obj_id, name), finalizer in list(bindable_properties.items()):
+        if obj_id in object_ids:
             del bindable_properties[(obj_id, name)]
+            finalizer.detach()
 
 
 def reset() -> None:

tests/test_binding.py

@@ -1,3 +1,4 @@
+import weakref
 from typing import Dict, Optional, Tuple
 
 from selenium.webdriver.common.keys import Keys
@@ -125,3 +126,35 @@ class TestClass:
     assert len(binding.bindings) == 2
     assert len(binding.active_links) == 1
     assert binding.active_links[0][1] == 'not_bindable'
+
+
+def test_automatic_cleanup(screen: Screen):
+    class Model:
+        value = binding.BindableProperty()
+
+        def __init__(self, value: str) -> None:
+            self.value = value
+
+    def create_model_and_label(value: str) -> Tuple[Model, weakref.ref, ui.label]:
+        model = Model(value)
+        label = ui.label(value).bind_text(model, 'value')
+        return id(model), weakref.ref(model), label
+
+    model_id1, ref1, label1 = create_model_and_label('first label')
+    model_id2, ref2, _label2 = create_model_and_label('second label')
+
+    def is_alive(ref: weakref.ref) -> bool:
+        return ref() is not None
+
+    def has_bindable_property(model_id: int) -> bool:
+        return any(obj_id == model_id for obj_id, _ in binding.bindable_properties)
+
+    screen.open('/')
+    screen.should_contain('first label')
+    screen.should_contain('second label')
+    assert is_alive(ref1) and has_bindable_property(model_id1)
+    assert is_alive(ref2) and has_bindable_property(model_id2)
+
+    binding.remove([label1])
+    assert not is_alive(ref1) and not has_bindable_property(model_id1)
+    assert is_alive(ref2) and has_bindable_property(model_id2)