diff --git a/bec_widgets/applications/companion_app.py b/bec_widgets/applications/companion_app.py index 43524194..2b4de2fb 100644 --- a/bec_widgets/applications/companion_app.py +++ b/bec_widgets/applications/companion_app.py @@ -198,8 +198,14 @@ class GUIServer: def stop_dispatcher(): if self.dispatcher: - self.dispatcher.stop_cli_server() + # disconnect_all() must run BEFORE stop_cli_server(). stop_cli_server() + # -> RPCServer.shutdown() historically tore the client down, which nulls + # the connector's listener thread; a later disconnect_all() would then + # early-return in ManagedRedisConnection.unregister and leave subscriptions + # alive. This ordering matches BECConnector.terminate (disconnect_all -> + # stop_cli_server). self.dispatcher.disconnect_all() + self.dispatcher.stop_cli_server() self._run_shutdown_step("close_launcher_window", close_launcher_window) self._run_shutdown_step("stop_pylsp_server", stop_pylsp_server) diff --git a/bec_widgets/applications/launch_window.py b/bec_widgets/applications/launch_window.py index 4ae1ee5a..6bf090e0 100644 --- a/bec_widgets/applications/launch_window.py +++ b/bec_widgets/applications/launch_window.py @@ -741,6 +741,10 @@ class LaunchWindow(BECMainWindow): return event.accept() + # Chain to the base implementation so BECWidget.cleanup() runs on the normal + # close path (not only via WA_DeleteOnClose / DeferredDelete). Keeps this method + # a correct closeEvent template. + super().closeEvent(event) if __name__ == "__main__": # pragma: no cover diff --git a/bec_widgets/utils/bec_connector.py b/bec_widgets/utils/bec_connector.py index c836e348..602561c9 100644 --- a/bec_widgets/utils/bec_connector.py +++ b/bec_widgets/utils/bec_connector.py @@ -155,7 +155,14 @@ class BECConnector: client.shutdown() BECConnector.EXIT_HANDLERS[self.client] = terminate - QApplication.instance().aboutToQuit.connect(terminate) + app = QApplication.instance() + if app is not None: + app.aboutToQuit.connect(terminate) + else: + logger.warning( + "No QApplication instance available; skipping aboutToQuit " + "registration for BEC client teardown handler." + ) if config: self.config = config diff --git a/bec_widgets/utils/bec_dispatcher.py b/bec_widgets/utils/bec_dispatcher.py index adf0f1df..0a0ad4be 100644 --- a/bec_widgets/utils/bec_dispatcher.py +++ b/bec_widgets/utils/bec_dispatcher.py @@ -416,10 +416,13 @@ class BECDispatcher: def stop_cli_server(self): """ - Stop the CLI server. + Stop the CLI server. Idempotent: application teardown has several owners + (``GUIServer.shutdown`` and the generic ``BECConnector.terminate`` exit + handler both run on ``aboutToQuit``), so a second stop is an expected + no-op, not an error. """ if self.cli_server is None: - logger.error("Cannot stop CLI server without starting it first") + logger.debug("CLI server already stopped or never started") return self.cli_server.shutdown() self.cli_server = None diff --git a/bec_widgets/utils/rpc_server.py b/bec_widgets/utils/rpc_server.py index 4f870a23..eff7798d 100644 --- a/bec_widgets/utils/rpc_server.py +++ b/bec_widgets/utils/rpc_server.py @@ -531,8 +531,15 @@ class RPCServer: def shutdown(self): """ Shut the RPC server down: stop the heartbeat, release the dispatcher - subscription and the registry callback, and shut the client down. - Safe to call multiple times. + subscription and the registry callback. Safe to call multiple times. + + The BEC client is intentionally NOT shut down here: it is shared with the + dispatcher (and, in the companion app, with the exit ``terminate`` handler). + Shutting it down here would stop the connector's listener thread, so a later + ``dispatcher.disconnect_all()`` (which routes through + ``ManagedRedisConnection.unregister``) would early-return and silently leave + subscriptions alive. Client teardown is left to ``BECConnector.terminate`` / + the dispatcher fixture, which own the client lifecycle. """ self.status = messages.BECStatus.IDLE self._heartbeat_timer.stop() @@ -542,4 +549,3 @@ class RPCServer: ) self.rpc_register.remove_callback(self.broadcast_registry_update) logger.info("Succeeded in shutting down CLI server") - self.client.shutdown() diff --git a/tests/unit_tests/test_bec_connector.py b/tests/unit_tests/test_bec_connector.py index 0edffc3e..dda3aedb 100644 --- a/tests/unit_tests/test_bec_connector.py +++ b/tests/unit_tests/test_bec_connector.py @@ -1,5 +1,6 @@ # pylint: disable = no-name-in-module,missing-class-docstring, missing-module-docstring import time +from unittest import mock import pytest from qtpy.QtCore import QObject @@ -203,3 +204,18 @@ def test_bec_connector_export_settings(): config = {"my_str_property": "new_value"} widget.load_settings(config) assert widget.my_str_property == "new_value" + + +def test_bec_connector_terminate_registration_no_qapp_instance(qtbot): + """Constructing a BECConnector before a QApplication exists must not raise; the exit + handler is still registered, only the aboutToQuit wiring is skipped.""" + import bec_widgets.utils.bec_connector as m + + fresh_client = mock.MagicMock(name="fresh_client") + assert fresh_client not in BECConnector.EXIT_HANDLERS + try: + with mock.patch.object(m.QApplication, "instance", return_value=None): + BECConnectorQObject(client=fresh_client) # must not raise + assert fresh_client in BECConnector.EXIT_HANDLERS + finally: + BECConnector.EXIT_HANDLERS.pop(fresh_client, None) diff --git a/tests/unit_tests/test_bec_dispatcher.py b/tests/unit_tests/test_bec_dispatcher.py index c0ec3071..651b8e37 100644 --- a/tests/unit_tests/test_bec_dispatcher.py +++ b/tests/unit_tests/test_bec_dispatcher.py @@ -259,3 +259,16 @@ def test_qt_redis_connector_logs_rpc_before_qt_callback(monkeypatch): assert "request_id=dispatcher-request" in warning_message finally: connector.shutdown() + + +def test_stop_cli_server_is_idempotent(bec_dispatcher): + """Both GUIServer.shutdown and BECConnector.terminate stop the CLI server at + application exit; the second call must be a silent no-op, not an ERROR.""" + from unittest import mock + + from bec_widgets.utils import bec_dispatcher as bd_module + + with mock.patch.object(bd_module, "logger") as mock_logger: + bec_dispatcher.stop_cli_server() + bec_dispatcher.stop_cli_server() + mock_logger.error.assert_not_called() diff --git a/tests/unit_tests/test_launch_window.py b/tests/unit_tests/test_launch_window.py index d837836c..edefb0c7 100644 --- a/tests/unit_tests/test_launch_window.py +++ b/tests/unit_tests/test_launch_window.py @@ -224,19 +224,28 @@ def test_launch_window_closes(bec_launch_window, qtbot, connection_names, close_ else: conn = _launcher_child_connection(bec_launch_window, name) connections[name] = conn - close_event = mock.MagicMock() + # A real QCloseEvent (accepted by default) so closeEvent can chain to + # super().closeEvent(event); patch cleanup to assert it runs on the accept path and + # to avoid exercising real teardown of the shared fixture under --random-order. + from qtpy.QtGui import QCloseEvent + + close_event = QCloseEvent() with mock.patch.object( bec_launch_window.register, "list_all_connections", return_value=connections ): - with mock.patch.object(bec_launch_window, "hide") as mock_hide: + with ( + mock.patch.object(bec_launch_window, "hide") as mock_hide, + mock.patch.object(bec_launch_window, "cleanup") as mock_cleanup, + ): bec_launch_window.closeEvent(close_event) if close_called: mock_hide.assert_not_called() - close_event.accept.assert_called_once() + assert close_event.isAccepted() + mock_cleanup.assert_called_once() # cleanup runs on the normal close path else: mock_hide.assert_called_once() - close_event.accept.assert_not_called() - close_event.ignore.assert_called_once() + assert not close_event.isAccepted() # ignore() flips the default True->False + mock_cleanup.assert_not_called() def test_main_label_fits_tile_width(bec_launch_window, qtbot): diff --git a/tests/unit_tests/test_rpc_server.py b/tests/unit_tests/test_rpc_server.py index f21a5325..27b947a9 100644 --- a/tests/unit_tests/test_rpc_server.py +++ b/tests/unit_tests/test_rpc_server.py @@ -339,3 +339,37 @@ def test_rpc_register_remove_callback_is_noop_for_unknown(rpc_register=None): register = RPCRegister() register.remove_callback(lambda connections: None) # must not raise + + +def test_rpc_server_shutdown_does_not_shut_down_client(mocked_client): + """RPCServer.shutdown must not tear down the shared client: doing so nulls the + connector's listener thread and turns a later disconnect_all() into a silent no-op.""" + from unittest.mock import patch + + server = RPCServer(gui_id="no_client_teardown", client=mocked_client) + with patch.object(mocked_client, "shutdown") as client_shutdown: + server.shutdown() + client_shutdown.assert_not_called() + # idempotent + still no client shutdown + with patch.object(mocked_client, "shutdown") as client_shutdown2: + server.shutdown() + client_shutdown2.assert_not_called() + + +def test_gui_server_stop_dispatcher_disconnects_before_stopping_cli_server(gui_server): + """disconnect_all() must run before stop_cli_server() so subscriptions are released + while the connector's listener thread is still alive.""" + from unittest.mock import MagicMock, patch + + calls = [] + disp = MagicMock() + disp.disconnect_all.side_effect = lambda *a, **k: calls.append("disconnect_all") + disp.stop_cli_server.side_effect = lambda *a, **k: calls.append("stop_cli_server") + gui_server.dispatcher = disp + gui_server.launcher_window = MagicMock() + with ( + patch.object(companion_app_module.shiboken6, "isValid", return_value=True), + patch.object(companion_app_module.pylsp_server, "is_running", return_value=False), + ): + gui_server.shutdown() + assert calls == ["disconnect_all", "stop_cli_server"]