From 67de22bafe6b7ad1f344a84c73bf1f4a90ea460a Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 20 Nov 2019 15:25:06 -0500 Subject: [PATCH] bt2: make _ListenerHandle not hold a strong reference on the target object The bt2._ListenerHandle object currently holds a strong reference to the Python object on which the listener was added. This allows validating that a handle passed to the remove_destruction_listener method of an object mathces that object. However, keeping that strong reference also prevents the destruction of that target object. So, adding a destruction listener and keeping the handle around actually prevents the destruction from happening, making that destruction listener useless. Change it so the _ListenerHandle only keeps the address of the target object. This is enough to do the check described above. We must however invalidate the _ListenerHandle when the target object is destroyed, because another object could be later created at the same address. To achieve this, the handle object is bound to the destruction callback, so that we can invalidate it in _trace_destruction_listener_from_native / _trace_class_destruction_listener_from_native. The "del" statements in the tests were necessary before, otherwise the handles would keep the trace class / trace alive, and the destruction listeners would not get called. I removed them, so it now tests that keeping a listener handle doesn't keep the target object alive. Change-Id: I668cf29b5a6046a89d4eff73d322cb0cd83e5109 Signed-off-by: Simon Marchi Reviewed-on: https://review.lttng.org/c/babeltrace/+/2426 Reviewed-by: Philippe Proulx --- src/bindings/python/bt2/bt2/trace.py | 15 ++++++++++----- src/bindings/python/bt2/bt2/trace_class.py | 19 +++++++++++++------ src/bindings/python/bt2/bt2/utils.py | 10 ++++++++-- tests/bindings/python/bt2/test_trace.py | 3 --- tests/bindings/python/bt2/test_trace_class.py | 3 --- 5 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/bindings/python/bt2/bt2/trace.py b/src/bindings/python/bt2/bt2/trace.py index 5dcaa15c..61889906 100644 --- a/src/bindings/python/bt2/bt2/trace.py +++ b/src/bindings/python/bt2/bt2/trace.py @@ -166,9 +166,11 @@ class _TraceConst(object._SharedObject, collections.abc.Mapping): if not callable(listener): raise TypeError("'listener' parameter is not callable") + handle = utils._ListenerHandle(self.addr) + fn = native_bt.bt2_trace_add_destruction_listener listener_from_native = functools.partial( - _trace_destruction_listener_from_native, listener + _trace_destruction_listener_from_native, listener, handle ) status, listener_id = fn(self._ptr, listener_from_native) @@ -176,12 +178,14 @@ class _TraceConst(object._SharedObject, collections.abc.Mapping): status, 'cannot add destruction listener to trace object' ) - return utils._ListenerHandle(listener_id, self) + handle._set_listener_id(listener_id) + + return handle def remove_destruction_listener(self, listener_handle): utils._check_type(listener_handle, utils._ListenerHandle) - if listener_handle._obj.addr != self.addr: + if listener_handle._addr != self.addr: raise ValueError( 'This trace destruction listener does not match the trace object.' ) @@ -193,7 +197,7 @@ class _TraceConst(object._SharedObject, collections.abc.Mapping): self._ptr, listener_handle._listener_id ) utils._handle_func_status(status) - listener_handle._listener_id = None + listener_handle._invalidate() class _Trace(_TraceConst): @@ -263,6 +267,7 @@ class _Trace(_TraceConst): return stream -def _trace_destruction_listener_from_native(user_listener, trace_ptr): +def _trace_destruction_listener_from_native(user_listener, handle, trace_ptr): trace = _TraceConst._create_from_ptr_and_get_ref(trace_ptr) user_listener(trace) + handle._invalidate() diff --git a/src/bindings/python/bt2/bt2/trace_class.py b/src/bindings/python/bt2/bt2/trace_class.py index dc9439eb..94010d47 100644 --- a/src/bindings/python/bt2/bt2/trace_class.py +++ b/src/bindings/python/bt2/bt2/trace_class.py @@ -33,9 +33,12 @@ import functools import bt2 -def _trace_class_destruction_listener_from_native(user_listener, trace_class_ptr): +def _trace_class_destruction_listener_from_native( + user_listener, handle, trace_class_ptr +): trace_class = _TraceClass._create_from_ptr_and_get_ref(trace_class_ptr) user_listener(trace_class) + handle._invalidate() class _TraceClassConst(object._SharedObject, collections.abc.Mapping): @@ -100,22 +103,26 @@ class _TraceClassConst(object._SharedObject, collections.abc.Mapping): if not callable(listener): raise TypeError("'listener' parameter is not callable") - fn = native_bt.bt2_trace_class_add_destruction_listener + handle = utils._ListenerHandle(self.addr) + listener_from_native = functools.partial( - _trace_class_destruction_listener_from_native, listener + _trace_class_destruction_listener_from_native, listener, handle ) + fn = native_bt.bt2_trace_class_add_destruction_listener status, listener_id = fn(self._ptr, listener_from_native) utils._handle_func_status( status, 'cannot add destruction listener to trace class object' ) - return utils._ListenerHandle(listener_id, self) + handle._set_listener_id(listener_id) + + return handle def remove_destruction_listener(self, listener_handle): utils._check_type(listener_handle, utils._ListenerHandle) - if listener_handle._obj.addr != self.addr: + if listener_handle._addr != self.addr: raise ValueError( 'This trace class destruction listener does not match the trace class object.' ) @@ -129,7 +136,7 @@ class _TraceClassConst(object._SharedObject, collections.abc.Mapping): self._ptr, listener_handle._listener_id ) utils._handle_func_status(status) - listener_handle._listener_id = None + listener_handle._invalidate() class _TraceClass(_TraceClassConst): diff --git a/src/bindings/python/bt2/bt2/utils.py b/src/bindings/python/bt2/bt2/utils.py index 77ebe606..67017cd0 100644 --- a/src/bindings/python/bt2/bt2/utils.py +++ b/src/bindings/python/bt2/bt2/utils.py @@ -166,6 +166,12 @@ def _handle_func_status(status, msg=None): class _ListenerHandle: - def __init__(self, listener_id, obj): + def __init__(self, addr): + self._addr = addr + self._listener_id = None + + def _set_listener_id(self, listener_id): self._listener_id = listener_id - self._obj = obj + + def _invalidate(self): + self._listener_id = None diff --git a/tests/bindings/python/bt2/test_trace.py b/tests/bindings/python/bt2/test_trace.py index 4a1b1b61..70e70e75 100644 --- a/tests/bindings/python/bt2/test_trace.py +++ b/tests/bindings/python/bt2/test_trace.py @@ -168,9 +168,6 @@ class TraceTestCase(unittest.TestCase): trace.remove_destruction_listener(td_handle2) - del td_handle1 - del td_handle2 - self.assertEqual(num_trace_class_destroyed_calls, 0) self.assertEqual(num_trace_destroyed_calls, 0) diff --git a/tests/bindings/python/bt2/test_trace_class.py b/tests/bindings/python/bt2/test_trace_class.py index bd80586c..eca5274e 100644 --- a/tests/bindings/python/bt2/test_trace_class.py +++ b/tests/bindings/python/bt2/test_trace_class.py @@ -198,9 +198,6 @@ class TraceClassTestCase(unittest.TestCase): trace_class.remove_destruction_listener(handle2) - del handle1 - del handle2 - self.assertEqual(num_destruct_calls, 0) del trace_class -- 2.34.1