From 314e89ba38fb8599c2626851f452d7615d4280c1 Mon Sep 17 00:00:00 2001 From: Martin Stadler Date: Mon, 20 May 2024 17:25:10 +0200 Subject: [PATCH 1/7] Use weak references in dict/list mappping to avoid memory leak --- .../observable/observable_object.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/pydase/observer_pattern/observable/observable_object.py b/src/pydase/observer_pattern/observable/observable_object.py index 82f0e70..72aa1d1 100644 --- a/src/pydase/observer_pattern/observable/observable_object.py +++ b/src/pydase/observer_pattern/observable/observable_object.py @@ -4,6 +4,7 @@ from collections.abc import Iterable from typing import TYPE_CHECKING, Any, ClassVar, SupportsIndex from pydase.utils.helpers import parse_serialized_key +import weakref if TYPE_CHECKING: from pydase.observer_pattern.observer.observer import Observer @@ -88,19 +89,17 @@ class ObservableObject(ABC): if isinstance(value, list): if id(value) in self._list_mapping: # If the list `value` was already referenced somewhere else - new_value = self._list_mapping[id(value)] + new_value = self._list_mapping[id(value)]() else: # convert the builtin list into a ObservableList new_value = _ObservableList(original_list=value) - self._list_mapping[id(value)] = new_value elif isinstance(value, dict): if id(value) in self._dict_mapping: # If the dict `value` was already referenced somewhere else - new_value = self._dict_mapping[id(value)] + new_value = self._dict_mapping[id(value)]() else: - # convert the builtin list into a ObservableList + # convert the builtin dict into a ObservableDict new_value = _ObservableDict(original_dict=value) - self._dict_mapping[id(value)] = new_value if isinstance(new_value, ObservableObject): new_value.add_observer(self, attr_name_or_key) return new_value @@ -138,6 +137,10 @@ class _ObservableList(ObservableObject, list[Any]): list.__init__(self, self._original_list) for i, item in enumerate(self._original_list): super().__setitem__(i, self._initialise_new_objects(f"[{i}]", item)) + self._list_mapping[id(self._original_list)] = weakref.ref(self) + + def __del__(self): + self._list_mapping.pop(id(self._original_list)) def __setitem__(self, key: int, value: Any) -> None: # type: ignore[override] if hasattr(self, "_observers"): @@ -236,6 +239,10 @@ class _ObservableDict(ObservableObject, dict[str, Any]): dict.__init__(self) for key, value in self._original_dict.items(): self.__setitem__(key, self._initialise_new_objects(f'["{key}"]', value)) + self._dict_mapping[id(self._original_dict)] = weakref.ref(self) + + def __del__(self): + self._dict_mapping.pop(id(self._original_dict)) def __setitem__(self, key: str, value: Any) -> None: if not isinstance(key, str): From 9c5d133d65353fb28af9de56d8569d9c1a9c3073 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mose=20M=C3=BCller?= Date: Tue, 21 May 2024 10:51:13 +0200 Subject: [PATCH 2/7] fixes types --- .../observer_pattern/observable/observable_object.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/pydase/observer_pattern/observable/observable_object.py b/src/pydase/observer_pattern/observable/observable_object.py index 72aa1d1..4cbb755 100644 --- a/src/pydase/observer_pattern/observable/observable_object.py +++ b/src/pydase/observer_pattern/observable/observable_object.py @@ -1,10 +1,10 @@ import logging +import weakref from abc import ABC, abstractmethod from collections.abc import Iterable from typing import TYPE_CHECKING, Any, ClassVar, SupportsIndex from pydase.utils.helpers import parse_serialized_key -import weakref if TYPE_CHECKING: from pydase.observer_pattern.observer.observer import Observer @@ -13,8 +13,8 @@ logger = logging.getLogger(__name__) class ObservableObject(ABC): - _list_mapping: ClassVar[dict[int, "_ObservableList"]] = {} - _dict_mapping: ClassVar[dict[int, "_ObservableDict"]] = {} + _list_mapping: ClassVar[dict[int, weakref.ReferenceType["_ObservableList"]]] = {} + _dict_mapping: ClassVar[dict[int, weakref.ReferenceType["_ObservableDict"]]] = {} def __init__(self) -> None: if not hasattr(self, "_observers"): @@ -139,7 +139,7 @@ class _ObservableList(ObservableObject, list[Any]): super().__setitem__(i, self._initialise_new_objects(f"[{i}]", item)) self._list_mapping[id(self._original_list)] = weakref.ref(self) - def __del__(self): + def __del__(self) -> None: self._list_mapping.pop(id(self._original_list)) def __setitem__(self, key: int, value: Any) -> None: # type: ignore[override] @@ -241,7 +241,7 @@ class _ObservableDict(ObservableObject, dict[str, Any]): self.__setitem__(key, self._initialise_new_objects(f'["{key}"]', value)) self._dict_mapping[id(self._original_dict)] = weakref.ref(self) - def __del__(self): + def __del__(self) -> None: self._dict_mapping.pop(id(self._original_dict)) def __setitem__(self, key: str, value: Any) -> None: From f9a5352efee6f8f6a8fd4f62aadf09c4f627b92a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mose=20M=C3=BCller?= Date: Tue, 21 May 2024 12:52:17 +0200 Subject: [PATCH 3/7] moves lines adding weakref to mapping dict into _initialise_new_objects This groups together all the lines that add elements to or get elements from the mapping dicts. --- .../observer_pattern/observable/observable_object.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/pydase/observer_pattern/observable/observable_object.py b/src/pydase/observer_pattern/observable/observable_object.py index 4cbb755..aa39413 100644 --- a/src/pydase/observer_pattern/observable/observable_object.py +++ b/src/pydase/observer_pattern/observable/observable_object.py @@ -93,6 +93,9 @@ class ObservableObject(ABC): else: # convert the builtin list into a ObservableList new_value = _ObservableList(original_list=value) + + # Use weakref to allow the GC to collect unused objects + self._list_mapping[id(value)] = weakref.ref(new_value) elif isinstance(value, dict): if id(value) in self._dict_mapping: # If the dict `value` was already referenced somewhere else @@ -100,6 +103,9 @@ class ObservableObject(ABC): else: # convert the builtin dict into a ObservableDict new_value = _ObservableDict(original_dict=value) + + # Use weakref to allow the GC to collect unused objects + self._dict_mapping[id(value)] = weakref.ref(new_value) if isinstance(new_value, ObservableObject): new_value.add_observer(self, attr_name_or_key) return new_value @@ -137,7 +143,6 @@ class _ObservableList(ObservableObject, list[Any]): list.__init__(self, self._original_list) for i, item in enumerate(self._original_list): super().__setitem__(i, self._initialise_new_objects(f"[{i}]", item)) - self._list_mapping[id(self._original_list)] = weakref.ref(self) def __del__(self) -> None: self._list_mapping.pop(id(self._original_list)) @@ -239,7 +244,6 @@ class _ObservableDict(ObservableObject, dict[str, Any]): dict.__init__(self) for key, value in self._original_dict.items(): self.__setitem__(key, self._initialise_new_objects(f'["{key}"]', value)) - self._dict_mapping[id(self._original_dict)] = weakref.ref(self) def __del__(self) -> None: self._dict_mapping.pop(id(self._original_dict)) From 6a894b61542a3e58628ba6b88df57e79aa3b12e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mose=20M=C3=BCller?= Date: Tue, 21 May 2024 13:39:41 +0200 Subject: [PATCH 4/7] adds test for dict/list garbage collection --- .../observable/test_observable_object.py | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/observer_pattern/observable/test_observable_object.py b/tests/observer_pattern/observable/test_observable_object.py index 6c9eb29..74c8a47 100644 --- a/tests/observer_pattern/observable/test_observable_object.py +++ b/tests/observer_pattern/observable/test_observable_object.py @@ -311,3 +311,51 @@ def test_list_remove(caplog: pytest.LogCaptureFixture) -> None: # checks if observer key was updated correctly (was index 1) other_observable_instance_2.greeting = "Ciao" assert "'my_list[0].greeting' changed to 'Ciao'" in caplog.text + + +def test_list_garbage_collection() -> None: + """Makes sure that the GC collects lists that are not referenced anymore.""" + + import gc + import json + + list_json = """ + [1] + """ + + class MyObservable(Observable): + def __init__(self) -> None: + super().__init__() + self.list_attr = json.loads(list_json) + + observable = MyObservable() + list_mapping_length = len(observable._list_mapping) + observable.list_attr = json.loads(list_json) + + gc.collect() + assert len(observable._list_mapping) <= list_mapping_length + + +def test_dict_garbage_collection() -> None: + """Makes sure that the GC collects dicts that are not referenced anymore.""" + + import gc + import json + + dict_json = """ + { + "foo": "bar" + } + """ + + class MyObservable(Observable): + def __init__(self) -> None: + super().__init__() + self.dict_attr = json.loads(dict_json) + + observable = MyObservable() + dict_mapping_length = len(observable._dict_mapping) + observable.dict_attr = json.loads(dict_json) + + gc.collect() + assert len(observable._dict_mapping) <= dict_mapping_length From 8285a37a4cc48d8fce56cea30008059e10911c79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mose=20M=C3=BCller?= Date: Tue, 21 May 2024 13:43:13 +0200 Subject: [PATCH 5/7] updates version to v0.8.3 --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 163ce53..6304dcc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "pydase" -version = "0.8.2" +version = "0.8.3" description = "A flexible and robust Python library for creating, managing, and interacting with data services, with built-in support for web and RPC servers, and customizable features for diverse use cases." authors = ["Mose Mueller "] readme = "README.md" From ba9dbc03f1f866428da99ef99e963854b3f07456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mose=20M=C3=BCller?= Date: Tue, 21 May 2024 14:03:21 +0200 Subject: [PATCH 6/7] removes attribute key from observers dict if list of observers is empty --- src/pydase/observer_pattern/observable/observable_object.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/pydase/observer_pattern/observable/observable_object.py b/src/pydase/observer_pattern/observable/observable_object.py index aa39413..d749ef5 100644 --- a/src/pydase/observer_pattern/observable/observable_object.py +++ b/src/pydase/observer_pattern/observable/observable_object.py @@ -32,6 +32,10 @@ class ObservableObject(ABC): if attribute in self._observers: self._observers[attribute].remove(observer) + # remove attribute key from observers dict if list of observers is empty + if not self._observers[attribute]: + del self._observers[attribute] + @abstractmethod def _remove_observer_if_observable(self, name: str) -> None: """Removes the current object as an observer from an observable attribute. From 9c3c92361b2558784ca1bdf0ff39f880ee056bc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mose=20M=C3=BCller?= Date: Tue, 21 May 2024 14:03:25 +0200 Subject: [PATCH 7/7] updates tests --- .../observable/test_observable_dict.py | 4 +--- .../observable/test_observable_object.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/tests/observer_pattern/observable/test_observable_dict.py b/tests/observer_pattern/observable/test_observable_dict.py index e29dc94..f06c00d 100644 --- a/tests/observer_pattern/observable/test_observable_dict.py +++ b/tests/observer_pattern/observable/test_observable_dict.py @@ -138,7 +138,6 @@ def test_removed_observer_on_class_dict_attr(caplog: pytest.LogCaptureFixture) - caplog.clear() assert nested_instance._observers == { - '["nested"]': [], "nested_attr": [instance], } @@ -172,7 +171,6 @@ def test_removed_observer_on_instance_dict_attr( caplog.clear() assert nested_instance._observers == { - '["nested"]': [], "nested_attr": [instance], } @@ -211,6 +209,6 @@ def test_pop(caplog: pytest.LogCaptureFixture) -> None: instance = MyObservable() MyObserver(instance) assert instance.dict_attr.pop("nested") == nested_instance - assert nested_instance._observers == {'["nested"]': []} + assert nested_instance._observers == {} assert f"'dict_attr' changed to '{instance.dict_attr}'" in caplog.text diff --git a/tests/observer_pattern/observable/test_observable_object.py b/tests/observer_pattern/observable/test_observable_object.py index 74c8a47..a5b6d6b 100644 --- a/tests/observer_pattern/observable/test_observable_object.py +++ b/tests/observer_pattern/observable/test_observable_object.py @@ -81,11 +81,21 @@ def test_removed_observer_on_class_list_attr(caplog: pytest.LogCaptureFixture) - instance = MyObservable() MyObserver(instance) + + assert nested_instance._observers == { + "[0]": [instance.changed_list_attr], + "nested_attr": [instance], + } + instance.changed_list_attr[0] = "Ciao" assert "'changed_list_attr[0]' changed to 'Ciao'" in caplog.text caplog.clear() + assert nested_instance._observers == { + "nested_attr": [instance], + } + instance.nested_attr.name = "Hi" assert "'nested_attr.name' changed to 'Hi'" in caplog.text @@ -115,6 +125,10 @@ def test_removed_observer_on_instance_list_attr( assert "'changed_list_attr[0]' changed to 'Ciao'" in caplog.text caplog.clear() + assert nested_instance._observers == { + "nested_attr": [instance], + } + instance.nested_attr.name = "Hi" assert "'nested_attr.name' changed to 'Hi'" in caplog.text