Repository: canonical/operator
Pull request #1142 (2024-03-01 15:53:10)

fix(framework): add warning on lost observer weakref
So this is a tricky one.

At times charmers are tempted to write:
```
        class MyObj(ops.Object):
            def __init__(self, charm, *args: typing.Any):
                super().__init__(charm, "obj")
                framework.observe(charm.on.start, self._on_start)

            def _on_start(self, _):
                raise RuntimeError()  # never reached!!! 


        class MyCharm(ops.CharmBase):
            def __init__(self, *args: typing.Any):
                super().__init__(*args)
                MyObj(self)  # not assigned!
                framework.observe(self.on.start, self._on_start)

            def _on_start(self, event: ops.EventBase):
                pass  # is reached
```

and expect that both _on_start methods will be triggered, while due to some subtle implementation details, MyObj._on_start never fires because the notice is silently dropped if the weakref stored in Framework._observer has been gc'd between charm instantiation time and Framework._emit time.

Long story short, I'm not sure why that codepath is so permissive and we don't error out altogether, that is, I don't see a legitimate reason why _observer is a weakref dict instead of a regular dict, as that predates me by a bunch. Perhaps @jameinel has some more context there?

For now, I'd propose adding a warning there so that people have something to go on with when that happens. We can also consider raising an exception, or making _observer a regular dict if those are viable alternatives.

benhoyt: Hmm, I don't understand why this should be a weakref either. I would have expected an `observe` call would keep the object alive so it can call it.

@jameinel put up [this PR](https://github.com/canonical/operator/pull/56) back in Nov 2019 that changed this from a regular dict to a weakref dict. It explains that it was done, and that "it's safe", but not really why it was done:

> 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.

John, can you remember (or think of) why we would have made this a weakref dict?
benhoyt: In terms of this PR, if there's a good reason for this to be a weakref, I don't mind adding the warning, but I want to get to the bottom of why we use weakref at all first.
jameinel: So we intentionally didn't want just a ref from observe() to be the thing that holds objects resident in memory. Both because it is a bit weird for TestObject() to actually have a side effect, but also because of how the actual event structure works.
We are taking the "Object Path" to figure out what to fire (especially in the case of something like defer()).
As such, we want to have a proper tree path where objects are referenced by their parent objects.
PietroPasotti: > So we intentionally didn't want just a ref from observe() to be the thing that holds objects resident in memory. Both because it is a bit weird for TestObject() to actually have a side effect, but also because of how the actual event structure works. We are taking the "Object Path" to figure out what to fire (especially in the case of something like defer()). As such, we want to have a proper tree path where objects are referenced by their parent objects.

This sounds reasonable and makes sense to me. At the same time it feels like we shouldn't allow refs to simply disappear from `_observer` without as much as a warning. Unless registering children Objects without holding references to them is a legitimate use case for some other reason.

Patch:
diff --git a/ops/framework.py b/ops/framework.py
index cc84436..645abdc 100644
--- a/ops/framework.py
+++ b/ops/framework.py
@@ -954,6 +954,13 @@ class Framework(Object):
                             # Regular call to the registered method.
                             custom_handler(event)
 
+            else:
+                logger.warning(
+                    f"Reference to ops.Object at path {observer_path} has been garbage collected "
+                    "between when the charm was initialised and when the event was emitted. "
+                    "Make sure sure you store a reference to the observer."
+                )
+
             if event.deferred:
                 deferred = True
             else:


Test patch:
diff --git a/test/test_charm.py b/test/test_charm.py
index 627d2d5..7ebbce2 100644
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -128,6 +128,30 @@ class TestCharm(unittest.TestCase):
         # check that the event has been seen by the observer
         self.assertIsInstance(charm.seen, ops.StartEvent)
 
+    def test_observer_not_referenced_warning(self):
+        class MyObj(ops.Object):
+            def __init__(self, charm: ops.CharmBase):
+                super().__init__(charm, "obj")
+                framework.observe(charm.on.start, self._on_start)
+
+            def _on_start(self, _: ops.StartEvent):
+                raise RuntimeError()  # never reached!
+
+        class MyCharm(ops.CharmBase):
+            def __init__(self, *args: typing.Any):
+                super().__init__(*args)
+                MyObj(self)  # not assigned!
+                framework.observe(self.on.start, self._on_start)
+
+            def _on_start(self, _: ops.StartEvent):
+                pass  # is reached
+
+        framework = self.create_framework()
+        c = MyCharm(framework)
+        with self.assertLogs() as logs:
+            c.on.start.emit()
+        assert any('Reference to ops.Object' in log for log in logs.output)
+
     def test_empty_action(self):
         meta = ops.CharmMeta.from_yaml('name: my-charm', '')
         self.assertEqual(meta.actions, {})