Use weak references in AggregateSensor

[bmerry]

This ensures that an aggregate sensor can be garbage collected once it's no longer explicitly referenced, even if its target sensor set is still live. It also makes __del__ more robust to exceptions occurring during __init__.

[coveralls]

Coverage increased (+0.1%) to 93.534% when pulling 6ea1a36 on NGC-442-aggregate-sensors-weakref into 82561af on ngc-442-aggregate-sensors.

[james-smith-za]

So I'll confess to still not quite understanding why this works. I decided to check whether it's necessary by running the unit tests without your weakref modifications, and I get the following:

    def test_aggregate_garbage_collection(self, ss, sensors):
        """Check that the aggregate can be garbage collected."""
        # Don't use the agg_sensor fixture, because pytest will hold its own
        # references to it.
        my_agg = MyAgg(target=ss, sensor_type=int, name="garbage")
        ss.add(sensors[1])
        weak = weakref.ref(my_agg)
        del my_agg
        # Some Python implementations need multiple rounds to garbage-collect
        # everything.
        for _ in range(5):
            gc.collect()
>       assert weak() is None  # i.e. my_agg was garbage-collected
E       AssertionError: assert <test_sensor_updated.MyAgg object at 0x7fba41553c70> is None
E        +  where <test_sensor_updated.MyAgg object at 0x7fba41553c70> = <weakref at 0x7fba414eef90; to 'MyAgg' at 0x7fba41553c70>()

So clearly the changes are necessary. It's going to take me a while to understand why though, and I don't want to hold up the process.

[james-smith-za]

This line reads rather weirdly.

[bmerry]

The __call__ on a weakref gives you the object it's referencing (or None if it is dead).

[james-smith-za]

Suggested change

	# "local variable '_my_agg' is assigned to but never used"
	# "local variable 'my_agg' is assigned to but never used"

Either that or add an underscore to the name of the variable that you create? I don't have a preference, they should probably just be consistent.

[bmerry]

Good catch - I'd tried using a leading underscore in case it shut up flake8 (it didn't) and copy-pasted the error from that version. Fixed now.

[bmerry]

So clearly the changes are necessary. It's going to take me a while to understand why though, and I don't want to hold up the process.

In case it helps: without the code changes, sensors[1] will have my_agg._update_aggregate as one of its observers. A bound method holds a reference to the instance its bound to, so sensors[1] indirectly references my_agg, keeping it alive. With the change, the observer becomes the wrapper closure defined by _weak_callback, which holds only a weak reference to my_agg.