fix(framework): add warning on lost observer weakref

[PietroPasotti]

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 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.

[benhoyt]

Some minor changes requested.

[benhoyt]

Suggested change

	def test_observe_nested_object_noninstantiated(self):
	def test_observer_not_referenced_warning(self):

Also, should this test live in test_framework.py, as the relevant code lives in framework.py?

[PietroPasotti]

I thought about it, but it felt way too complicated to implement it without referring to the charm object. If you look at the test immediately above it, I feel it makes sense to keep it here as they're very similar.
If you really want that that's a significant rework of the test.

[benhoyt]

Fair -- thanks!