Repository: canonical/operator
Pull request #56 (2019-11-26 09:28:32)

Change Framework._observer to be a WeakValueDictionary
This means that setting up an observe() won't keep your object alive
indefinitely. This is safe, because we don't ever iterate the _observer
slice, and we already handled the case where we have a registered
observer_path but no matching observer.


Patch:
diff --git a/.gitignore b/.gitignore
index bee8a64..36b14f6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,1 +1,2 @@
 __pycache__
+/sandbox
diff --git a/op/framework.py b/op/framework.py
old mode 100644
new mode 100755
index c39cf45..d139f10
--- a/op/framework.py
+++ b/op/framework.py
@@ -6,6 +6,7 @@ import sqlite3
 import collections
 import collections.abc
 import keyword
+import weakref
 
 
 class Handle:
@@ -371,8 +372,8 @@ class Framework(Object):
         self.meta = meta
         self.model = model
         self._event_count = 0
-        self._observers = []      # [(observer, method_name, parent_path, event_key)]
-        self._observer = {}       # {observer_path: observer}
+        self._observers = []      # [(observer_path, method_name, parent_path, event_key)]
+        self._observer = weakref.WeakValueDictionary()       # {observer_path: observer}
         self._type_registry = {}  # {(parent_path, kind): cls}
         self._type_known = set()  # {cls}
 
@@ -497,6 +498,7 @@ class Framework(Object):
         event_path = event.handle.path
         event_kind = event.handle.kind
         parent_path = event.handle.parent.path
+        # TODO Track observers by (parent_path, event_kind) rather than as a list of all observers. Avoiding linear search through all observers for every event
         for observer_path, method_name, _parent_path, _event_kind in self._observers:
             if _parent_path != parent_path:
                 continue
@@ -602,9 +604,6 @@ class BoundStoredState:
         self.__dict__["_data"] = data
         self.__dict__["_attr_name"] = attr_name
 
-        # TODO: Use weak references to make sure observers are garbage collected as a new observer will
-        # be created every time an instance of BoundStoredState is created.
-
         parent.framework.observe(parent.framework.on.commit, self._data)
 
     def __getattr__(self, key):
diff --git a/op/model.py b/op/model.py
index be9abad..09ea7ec 100644
--- a/op/model.py
+++ b/op/model.py
@@ -7,10 +7,10 @@ from subprocess import run, PIPE, CalledProcessError
 
 
 class Model:
-    def __init__(self, local_unit_name, relation_names, backend):
+    def __init__(self, unit_name, relation_names, backend):
         self._cache = ModelCache(backend)
         self._backend = backend
-        self.unit = self.get_unit(local_unit_name)
+        self.unit = self.get_unit(unit_name)
         self.app = self.unit.app
         self.relations = RelationMapping(relation_names, self.unit, self._backend, self._cache)
         self.config = ConfigData(self._backend)
@@ -70,7 +70,7 @@ class Application:
         self._backend = backend
         self._cache = cache
 
-        self.is_local = self.name == self._backend.local_app_name
+        self._is_our_app = self.name == self._backend.app_name
 
     def __repr__(self):
         return f'<{type(self).__module__}.{type(self).__name__} {self.name}>'
@@ -86,11 +86,19 @@ class Unit:
         self._backend = backend
         self._cache = cache
 
-        self.is_local = self.name == self._backend.local_unit_name
+        self._is_our_unit = self.name == self._backend.unit_name
 
     def __repr__(self):
         return f'<{type(self).__module__}.{type(self).__name__} {self.name}>'
 
+    def is_leader(self):
+        if self._is_our_unit:
+            # This value is not cached as it is not guaranteed to persist for the whole duration
+            # of a hook execution.
+            return self._backend.is_leader()
+        else:
+            raise RuntimeError(f"cannot determine leadership status for remote applications: {self}")
+
 class LazyMapping(Mapping, ABC):
     _lazy_data = None
 
@@ -120,8 +128,8 @@ class LazyMapping(Mapping, ABC):
 
 class RelationMapping(Mapping):
     """Map of relation names to lists of Relation instances."""
-    def __init__(self, relation_names, local_unit, backend, cache):
-        self._local_unit = local_unit
+    def __init__(self, relation_names, our_unit, backend, cache):
+        self._our_unit = our_unit
         self._backend = backend
         self._cache = cache
         self._data = {relation_name: None for relation_name in relation_names}
@@ -140,12 +148,12 @@ class RelationMapping(Mapping):
         if relation_list is None:
             relation_list = self._data[relation_name] = []
             for relation_id in self._backend.relation_ids(relation_name):
-                relation_list.append(Relation(relation_name, relation_id, self._local_unit, self._backend, self._cache))
+                relation_list.append(Relation(relation_name, relation_id, self._our_unit, self._backend, self._cache))
         return relation_list
 
 
 class Relation:
-    def __init__(self, relation_name, relation_id, local_unit, backend, cache):
+    def __init__(self, relation_name, relation_id, our_unit, backend, cache):
         self.name = relation_name
         self.id = relation_id
         self.app = None
@@ -159,16 +167,16 @@ class Relation:
         except RelationNotFound:
             # If the relation is dead, just treat it as if it has no remote units.
             pass
-        self.data = RelationData(self, local_unit, backend)
+        self.data = RelationData(self, our_unit, backend)
 
     def __repr__(self):
         return f'<{type(self).__module__}.{type(self).__name__} {self.name}:{self.id}>'
 
 
 class RelationData(Mapping):
-    def __init__(self, relation, local_unit, backend):
+    def __init__(self, relation, our_unit, backend):
         self.relation = weakref.proxy(relation)
-        self._data = {local_unit: RelationUnitData(self.relation, local_unit, True, backend)}
+        self._data = {our_unit: RelationUnitData(self.relation, our_unit, True, backend)}
         self._data.update({unit: RelationUnitData(self.relation, unit, False, backend) for unit in self.relation.units})
 
     def __contains__(self, key):
@@ -251,8 +259,8 @@ class RelationNotFound(ModelError):
 
 class ModelBackend:
     def __init__(self):
-        self.local_unit_name = os.environ['JUJU_UNIT_NAME']
-        self.local_app_name = self.local_unit_name.split('/')[0]
+        self.unit_name = os.environ['JUJU_UNIT_NAME']
+        self.app_name = self.unit_name.split('/')[0]
 
     def _run(self, *args):
         result = run(args + ('--format=json',), stdout=PIPE, stderr=PIPE, check=True)
@@ -302,3 +310,6 @@ class ModelBackend:
 
     def config_get(self):
         return self._run('config-get')
+
+    def is_leader(self):
+        return self._run('is-leader')


Test patch:
diff --git a/test/test_framework.py b/test/test_framework.py
index 5fd59ec..ee2c6f0 100755
--- a/test/test_framework.py
+++ b/test/test_framework.py
@@ -3,6 +3,7 @@
 import unittest
 import tempfile
 import shutil
+import gc
 
 from pathlib import Path
 
@@ -289,6 +290,37 @@ class TestFramework(unittest.TestCase):
         #
         self.assertEqual(obs.seen, ["on_foo:foo=2", "on_foo:foo=2"])
 
+    def test_weak_observer(self):
+        framework = self.create_framework()
+
+        observed_events = []
+
+        class MyEvent(EventBase):
+            pass
+
+        class MyEvents(EventsBase):
+            foo = Event(MyEvent)
+
+        class MyNotifier(Object):
+            on = MyEvents()
+
+        class MyObserver(Object):
+            def on_foo(self, event):
+                observed_events.append("foo")
+
+        pub = MyNotifier(framework, "1")
+        obs = MyObserver(framework, "2")
+
+        framework.observe(pub.on.foo, obs)
+        pub.on.foo.emit()
+        self.assertEqual(observed_events, ["foo"])
+        # Now delete the observer, and note that when we emit the event, it
+        # doesn't update the local slice again
+        del obs
+        gc.collect()
+        pub.on.foo.emit()
+        self.assertEqual(observed_events, ["foo"])
+
     def test_events_base(self):
         framework = self.create_framework()
 
diff --git a/test/test_model.py b/test/test_model.py
index b04274a..3b87d94 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -18,11 +18,8 @@ class FakeModelBackend:
     def __init__(self):
         self.relation_set_calls = []
 
-        self.local_unit_name = 'myapp/0'
-        self.local_app_name = 'myapp'
-
-    def local_app_name(self):
-        return 'myapp'
+        self.unit_name = 'myapp/0'
+        self.app_name = 'myapp'
 
     def relation_ids(self, relation_name):
         return {
@@ -72,13 +69,15 @@ class FakeModelBackend:
             'qux': True,
         }
 
-
 class TestModel(unittest.TestCase):
 
     def setUp(self):
         self.backend = FakeModelBackend()
         self.model = op.model.Model('myapp/0', ['db0', 'db1', 'db2'], self.backend)
 
+        os.environ['JUJU_UNIT_NAME'] = 'myapp/0'
+        self.addCleanup(os.environ.pop, 'JUJU_UNIT_NAME')
+
     def test_model(self):
         self.assertIs(self.model.app, self.model.unit.app)
 
@@ -102,14 +101,14 @@ class TestModel(unittest.TestCase):
         with self.assertRaises(op.model.TooManyRelatedApps):
             self.model.get_relation('db2')
 
-    def test_remote_units_is_local(self):
+    def test_remote_units_is_our(self):
         for u in self.model.get_relation('db1').units:
-            self.assertFalse(u.is_local)
-            self.assertFalse(u.app.is_local)
+            self.assertFalse(u._is_our_unit)
+            self.assertFalse(u.app._is_our_app)
 
-    def test_local_unit_is_local(self):
-        self.assertTrue(self.model.unit.is_local)
-        self.assertTrue(self.model.unit.app.is_local)
+    def test_our_unit_is_our(self):
+        self.assertTrue(self.model.unit._is_our_unit)
+        self.assertTrue(self.model.unit.app._is_our_app)
 
     def test_relation_data(self):
         random_unit = self.model._cache.get(op.model.Unit, 'randomunit/0')
@@ -128,7 +127,7 @@ class TestModel(unittest.TestCase):
         self.assertEqual(self.backend.relation_set_calls, [])
         self.assertNotIn('foo', rel_db1.data[remoteapp1_0])
 
-    def test_relation_data_modify_local(self):
+    def test_relation_data_modify_our(self):
         rel_db1 = self.model.get_relation('db1')
         # Force memory cache to be loaded.
         self.assertIn('host', rel_db1.data[self.model.unit])
@@ -175,6 +174,43 @@ class TestModel(unittest.TestCase):
             # Confirm that we cannot modify config values.
             self.model.config['foo'] = 'bar'
 
+    def test_is_leader(self):
+        self.backend = op.model.ModelBackend()
+        self.model = op.model.Model('myapp/0', ['db0', 'db1', 'db2'], self.backend)
+
+        def check_remote_units():
+            fake_script(self, 'relation-ids',
+                        """[ "$1" = db1 ] && echo '["db1:4"]' || echo '[]'""")
+
+            fake_script(self, 'relation-list',
+                        """[ "$2" = 4 ] && echo '["remoteapp1/0", "remoteapp1/1"]' || exit 2""")
+
+            # Cannot determine leadership for remote units.
+            for u in self.model.get_relation('db1').units:
+                with self.assertRaises(RuntimeError):
+                    u.is_leader()
+
+        fake_script(self, 'is-leader', 'echo true')
+        self.assertTrue(self.model.unit.is_leader())
+
+        check_remote_units()
+
+        self.backend = op.model.ModelBackend()
+        self.model = op.model.Model('myapp/0', ['db0', 'db1', 'db2'], self.backend)
+
+        fake_script(self, 'is-leader', 'echo false')
+        self.assertFalse(self.model.unit.is_leader())
+
+        check_remote_units()
+
+        self.assertEqual(fake_script_calls(self), [
+            ['is-leader', '--format=json'],
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json'],
+            ['is-leader', '--format=json'],
+            ['relation-ids', 'db1', '--format=json'],
+            ['relation-list', '-r', '4', '--format=json'],
+        ])
 
 def fake_script(test_case, name, content):
     if not hasattr(test_case, 'fake_script_path'):