Repository: zauberzeug/nicegui
Pull request #4122 (2024-12-18 11:27:56)

Automatic unregistering of `BindableProperty` objects
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.*

andybayer: Yes, I tried replacing the `Dict` with a `Set` and everything worked as expected. I started to implement this fix, but suddenly I thought that the `Set` also needs to be cleaned up, or else deleted items will clutter it up. So I thought an automatic cleanup action was needed. 

I added the cleanup action as a new value to the `Dict`. Actually, I think it's not the best design because the `Dict` now has two jobs. It serves as a quick lookup of whether an object-attribute-pair is a `BindableProperty` and also as a storage for abortable cleanup actions. Perhaps we should not store a reference to the cleanup action at all, since it does no harm to run it after the object has already been manually removed with `binding.remove`?
andybayer: So what would you suggest as the next step(s) @falkoschindler?

Shall I change something?

Patch:
diff --git a/nicegui/binding.py b/nicegui/binding.py
index d047d958..ee857420 100644
--- a/nicegui/binding.py
+++ b/nicegui/binding.py
@@ -3,6 +3,7 @@ from __future__ import annotations
 import asyncio
 import dataclasses
 import time
+import weakref
 from collections import defaultdict
 from typing import (
     TYPE_CHECKING,
@@ -32,7 +33,7 @@ if TYPE_CHECKING:
 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 @@ class BindableProperty:
         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:


Test patch:
diff --git a/tests/test_binding.py b/tests/test_binding.py
index 2c0ad2e3..19d539dd 100644
--- a/tests/test_binding.py
+++ b/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 @@ def test_bindable_dataclass(screen: Screen):
     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)