diff --git a/bec_widgets/applications/launch_window.py b/bec_widgets/applications/launch_window.py index 349db130..4ae1ee5a 100644 --- a/bec_widgets/applications/launch_window.py +++ b/bec_widgets/applications/launch_window.py @@ -282,7 +282,7 @@ class LaunchWindow(BECMainWindow): self._update_theme() self.register = RPCRegister() - self.register.callbacks.append(self._turn_off_the_lights) + self.register.add_callback(self._turn_off_the_lights) self.register.broadcast() if launch_gui_class and launch_gui_id: @@ -722,6 +722,14 @@ class LaunchWindow(BECMainWindow): if self.app: self.app.setQuitOnLastWindowClosed(True) # type: ignore + def cleanup(self): + """ + Deregister the turn-off-the-lights callback before teardown so later + registry broadcasts never call into a dying launcher. + """ + self.register.remove_callback(self._turn_off_the_lights) + super().cleanup() + def closeEvent(self, event): """ Close the launcher window. diff --git a/bec_widgets/utils/rpc_register.py b/bec_widgets/utils/rpc_register.py index f49409f7..bae14702 100644 --- a/bec_widgets/utils/rpc_register.py +++ b/bec_widgets/utils/rpc_register.py @@ -7,6 +7,7 @@ from weakref import WeakValueDictionary import shiboken6 as shb from bec_lib.logger import bec_logger +from louie.saferef import safe_ref from qtpy.QtCore import QObject if TYPE_CHECKING: # pragma: no cover @@ -147,14 +148,33 @@ class RPCRegister: def broadcast(self): """ - Broadcast the update to all the callbacks. + Broadcast the update to all the callbacks. Callbacks whose owners have + been garbage collected — or whose owning QObject's C++ side has been + destroyed while the Python wrapper is still referenced — are pruned + instead of being called. """ if self._skip_broadcast: return connections = self.list_all_connections() - for callback in self.callbacks: + dead_refs = [] + for callback_ref in list(self.callbacks): + callback = callback_ref() + if callback is None: + dead_refs.append(callback_ref) + continue + owner = getattr(callback, "__self__", None) + if isinstance(owner, QObject) and not shb.isValid(owner): + # Bound method of a destroyed widget (Python wrapper still alive, + # e.g. during shutdown): calling it would raise on any Qt access. + dead_refs.append(callback_ref) + continue callback(connections) + for ref in dead_refs: + try: + self.callbacks.remove(ref) + except ValueError: + pass def object_is_registered(self, obj: BECConnector) -> bool: """ @@ -172,22 +192,30 @@ class RPCRegister: """ Add a callback that will be called whenever the registry is updated. + Callbacks are stored as weak references (safe for bound methods): the + register never keeps a callback's owner alive. Registering the same + callback twice is a no-op, so broadcasts are delivered exactly once + per registered callback. + Args: callback(Callable[[dict], None]): The callback to be added. It should accept a dictionary of all the registered RPC objects as an argument. """ - self.callbacks.append(callback) + callback_ref = safe_ref(callback) + if callback_ref not in self.callbacks: + self.callbacks.append(callback_ref) def remove_callback(self, callback: Callable[[dict], None]): """ Remove a previously added registry-update callback. Removing a callback - that is not registered is a no-op. + that is not registered (or removing it twice) is a no-op. Args: callback(Callable[[dict], None]): The callback to be removed. """ + callback_ref = safe_ref(callback) try: - self.callbacks.remove(callback) + self.callbacks.remove(callback_ref) except ValueError: pass diff --git a/tests/unit_tests/test_bec_connector.py b/tests/unit_tests/test_bec_connector.py index 803ef92b..0edffc3e 100644 --- a/tests/unit_tests/test_bec_connector.py +++ b/tests/unit_tests/test_bec_connector.py @@ -158,9 +158,12 @@ def test_bec_widget_cleanup_broadcasts_after_children_are_unregistered(mocked_cl qtbot.addWidget(parent) observed_connections = [] - parent.rpc_register.callbacks.append( - lambda connections: observed_connections.append(set(connections)) - ) + + # Keep a strong reference: registry callbacks are weakly referenced. + def _observe_connections(connections): + observed_connections.append(set(connections)) + + parent.rpc_register.add_callback(_observe_connections) parent.close() diff --git a/tests/unit_tests/test_launch_window.py b/tests/unit_tests/test_launch_window.py index 28fd0988..d837836c 100644 --- a/tests/unit_tests/test_launch_window.py +++ b/tests/unit_tests/test_launch_window.py @@ -266,3 +266,14 @@ def test_main_label_point_size_uniform(bec_launch_window): """ point_sizes = {tile.main_label.font().pointSize() for tile in bec_launch_window.tiles.values()} assert len(point_sizes) == 1, f"Non-uniform main-label point sizes: {point_sizes}" + + +def test_launch_window_cleanup_deregisters_registry_callback(bec_launch_window): + """The turn-off-the-lights callback must not survive the launcher's cleanup: + a later registry broadcast would call into a dying window (shutdown scenario).""" + from louie.saferef import safe_ref + + reg = bec_launch_window.register + assert safe_ref(bec_launch_window._turn_off_the_lights) in reg.callbacks + bec_launch_window.cleanup() + assert safe_ref(bec_launch_window._turn_off_the_lights) not in reg.callbacks diff --git a/tests/unit_tests/test_rpc_register.py b/tests/unit_tests/test_rpc_register.py index 5250d138..9654b87d 100644 --- a/tests/unit_tests/test_rpc_register.py +++ b/tests/unit_tests/test_rpc_register.py @@ -50,3 +50,83 @@ def test_reset_singleton(rpc_register): assert len(all_connections) == 0 assert all_connections == {} + + +class _CallbackOwner: + """Owner of a bound-method registry callback for lifecycle tests.""" + + def __init__(self): + self.received = [] + + def on_update(self, connections): + self.received.append(dict(connections)) + + +def test_register_callback_receives_broadcast(rpc_register): + owner = _CallbackOwner() + rpc_register.add_callback(owner.on_update) + + rpc_register.broadcast() + assert len(owner.received) == 1 + + +def test_duplicate_callback_registration_is_deduplicated(rpc_register): + """Regression test for BW-009: registering the same bound method twice + must deliver each broadcast exactly once.""" + owner = _CallbackOwner() + callbacks_before = len(rpc_register.callbacks) + + rpc_register.add_callback(owner.on_update) + rpc_register.add_callback(owner.on_update) + assert len(rpc_register.callbacks) == callbacks_before + 1 + + rpc_register.broadcast() + assert len(owner.received) == 1 + + +def test_remove_callback_is_idempotent(rpc_register): + owner = _CallbackOwner() + callbacks_before = len(rpc_register.callbacks) + + rpc_register.add_callback(owner.on_update) + rpc_register.remove_callback(owner.on_update) + assert len(rpc_register.callbacks) == callbacks_before + + # Repeated removal and removing an unknown callback are no-ops. + rpc_register.remove_callback(owner.on_update) + rpc_register.remove_callback(_CallbackOwner().on_update) + assert len(rpc_register.callbacks) == callbacks_before + + rpc_register.broadcast() + assert owner.received == [] + + +def test_bound_method_identity_across_method_objects(rpc_register): + """Two bound-method objects for the same method of the same instance must + be treated as the same callback (remove works with a fresh method object).""" + owner = _CallbackOwner() + rpc_register.add_callback(owner.on_update) + # 'owner.on_update' here creates a *new* bound-method object. + rpc_register.remove_callback(owner.on_update) + + rpc_register.broadcast() + assert owner.received == [] + + +def test_callback_does_not_keep_owner_alive(rpc_register): + """Regression test for BW-009: the register must not keep callback owners + alive, and dead callbacks must be pruned on the next broadcast.""" + import gc + import weakref + + owner = _CallbackOwner() + rpc_register.add_callback(owner.on_update) + ref = weakref.ref(owner) + callbacks_with_owner = len(rpc_register.callbacks) + + del owner + gc.collect() + assert ref() is None, "register must not hold a strong reference to the owner" + + rpc_register.broadcast() # must not raise; prunes the dead reference + assert len(rpc_register.callbacks) == callbacks_with_owner - 1