diff --git a/datalab/control/remote.py b/datalab/control/remote.py index 35ceae36b..40275fb69 100644 --- a/datalab/control/remote.py +++ b/datalab/control/remote.py @@ -50,6 +50,8 @@ def method_wrapper(*args, **kwargs): """Decorator wrapper function""" self: RemoteServer = args[0] # extracting 'self' from method arguments self.is_ready = False + self.exception = None + self.result = None output = func(*args, **kwargs) while not self.is_ready: QC.QCoreApplication.processEvents() @@ -116,33 +118,37 @@ def __init__(self, win: DLMainWindow) -> None: self.win = win self.result = None self.exception = None + self.__window_ready = threading.Event() win.SIG_READY.connect(self.datalab_is_ready) win.SIG_CLOSING.connect(self.shutdown_server) self.SIG_CLOSE_APP.connect(win.close) self.SIG_RAISE_WINDOW.connect(win.raise_window) - self.SIG_ADD_OBJECT.connect(win.add_object) - self.SIG_SET_OBJECT.connect(win.set_object) - self.SIG_ADD_GROUP.connect(win.add_group) - self.SIG_LOAD_FROM_FILES.connect(win.load_from_files) - self.SIG_LOAD_FROM_DIRECTORY.connect(win.load_from_directory) - self.SIG_SELECT_OBJECTS.connect(win.select_objects) - self.SIG_SELECT_GROUPS.connect(win.select_groups) - self.SIG_SELECT_ALL_GROUPS.connect(lambda panel: win.select_groups(None, panel)) - self.SIG_DELETE_METADATA.connect(win.delete_metadata) - self.SIG_SWITCH_TO_PANEL.connect(win.set_current_panel) - self.SIG_TOGGLE_AUTO_REFRESH.connect(win.toggle_auto_refresh) - self.SIG_TOGGLE_SHOW_TITLES.connect(win.toggle_show_titles) - self.SIG_RESET_ALL.connect(win.reset_all) - self.SIG_REMOVE_OBJECT.connect(win.remove_object) - self.SIG_SAVE_TO_H5.connect(win.save_to_h5_file) - self.SIG_OPEN_H5.connect(win.open_h5_files) - self.SIG_IMPORT_H5.connect(win.import_h5_file) - self.SIG_LOAD_H5_WORKSPACE.connect(win.load_h5_workspace) - self.SIG_SAVE_H5_WORKSPACE.connect(win.save_h5_workspace) - self.SIG_CALC.connect(win.calc) - self.SIG_RUN_MACRO.connect(win.run_macro) - self.SIG_STOP_MACRO.connect(win.stop_macro) - self.SIG_IMPORT_MACRO_FROM_FILE.connect(win.import_macro_from_file) + for signal, command in ( + (self.SIG_ADD_OBJECT, win.add_object), + (self.SIG_SET_OBJECT, win.set_object), + (self.SIG_ADD_GROUP, win.add_group), + (self.SIG_LOAD_FROM_FILES, win.load_from_files), + (self.SIG_LOAD_FROM_DIRECTORY, win.load_from_directory), + (self.SIG_SELECT_OBJECTS, win.select_objects), + (self.SIG_SELECT_GROUPS, win.select_groups), + (self.SIG_SELECT_ALL_GROUPS, functools.partial(win.select_groups, None)), + (self.SIG_DELETE_METADATA, win.delete_metadata), + (self.SIG_SWITCH_TO_PANEL, win.set_current_panel), + (self.SIG_TOGGLE_AUTO_REFRESH, win.toggle_auto_refresh), + (self.SIG_TOGGLE_SHOW_TITLES, win.toggle_show_titles), + (self.SIG_RESET_ALL, win.reset_all), + (self.SIG_REMOVE_OBJECT, win.remove_object), + (self.SIG_SAVE_TO_H5, win.save_to_h5_file), + (self.SIG_OPEN_H5, win.open_h5_files), + (self.SIG_IMPORT_H5, win.import_h5_file), + (self.SIG_LOAD_H5_WORKSPACE, win.load_h5_workspace), + (self.SIG_SAVE_H5_WORKSPACE, win.save_h5_workspace), + (self.SIG_CALC, win.calc), + (self.SIG_RUN_MACRO, win.run_macro), + (self.SIG_STOP_MACRO, win.stop_macro), + (self.SIG_IMPORT_MACRO_FROM_FILE, win.import_macro_from_file), + ): + signal.connect(functools.partial(win.run_remote_command, command)) self.SIG_CALL_METHOD.connect(win.call_method_slot) def serve(self) -> None: @@ -157,11 +163,27 @@ def serve(self) -> None: self.register_functions(server) self.port = server.server_address[1] self.notify_port(self.port) + # Clients may already connect: their requests wait for the main window + self.__window_ready.wait() with execenv.context(xmlrpcport=self.port): server.serve_forever() + def notify_window_ready(self) -> None: + """Notify that the main window is fully set up. + + Requests are processed from then on: processing them earlier would run + commands on a partially built main window. + """ + self.__window_ready.set() + def shutdown_server(self) -> None: """Shutdown server""" + # A server still waiting for the main window must be able to stop + self.__window_ready.set() + # Release the remote call waiting for DataLab to be ready, if any (e.g. the + # one closing DataLab): the main window, blocked below until the request is + # done, could not signal it, hence a deadlock + self.is_ready = True if self.server is not None: self.server.shutdown() self.server = None diff --git a/datalab/gui/main.py b/datalab/gui/main.py index b7f40e79c..a92da45f5 100644 --- a/datalab/gui/main.py +++ b/datalab/gui/main.py @@ -106,7 +106,7 @@ from datalab.widgets.expandabletext import refresh_subdued_colors if TYPE_CHECKING: - from collections.abc import Iterator + from collections.abc import Callable, Iterator from typing import Any, Literal from datalab.gui.historysession_ops import SessionBehavior @@ -221,7 +221,8 @@ def _before_setup(self, console: bool) -> None: self.h5inputoutput = H5InputOutput(self) self.capsuleio = CapsuleInputOutput(self) - # Starting XML-RPC server thread + # Starting XML-RPC server thread: requests are processed once the main + # window is fully set up (see `_after_setup`) self.remote_server = RemoteServer(self) if Conf.rpc_server_enabled.get(): self.remote_server.SIG_SERVER_PORT.connect(self.xmlrpc_server_started) @@ -230,6 +231,11 @@ def _before_setup(self, console: bool) -> None: self.__register_plugins() self.webapi_actions = WebApiActions(self) + def _after_setup(self, console: bool) -> None: + """Process remote requests, the main window being fully set up.""" + super()._after_setup(console) + self.remote_server.notify_window_ready() + # ------API related to XML-RPC remote control @staticmethod def xmlrpc_server_started(port): @@ -667,6 +673,23 @@ def call_method_slot( self.remote_server.result = None self.remote_server.exception = exc + @remote_controlled + def run_remote_command(self, command: Callable[..., object], *args) -> None: + """Slot to run a command requested by a remote client in GUI thread. + + The exception raised by the command, if any, is stored in RemoteServer to + be re-raised to the client. Being remote controlled, this slot signals that + DataLab is ready only once the command is done and its exception stored. + + Args: + command: Main window method running the command + args: Command arguments + """ + try: + command(*args) + except Exception as exc: # pylint: disable=broad-except + self.remote_server.exception = exc + @remote_controlled def get_object_shapes( self, @@ -2518,12 +2541,7 @@ def add_object( panel_str = "image" else: raise TypeError(f"Unsupported object type {type(obj)}") - # A remote call can reach the main window before the History panel dock - # exists, since the XML-RPC server is started before the UI is built. historypanel = self.historypanel - if historypanel is None: - panel.add_object(obj, group_id, set_current) - return True historypanel.maybe_start_session_for_input(behavior=new_session_behavior) panel.add_object(obj, group_id, set_current) # Record a creation entry so objects added programmatically (plugins, diff --git a/datalab/tests/features/control/remote_requests_unit_test.py b/datalab/tests/features/control/remote_requests_unit_test.py new file mode 100644 index 000000000..01065397e --- /dev/null +++ b/datalab/tests/features/control/remote_requests_unit_test.py @@ -0,0 +1,101 @@ +# Copyright (c) DataLab Platform Developers, BSD 3-Clause license, see LICENSE file. + +""" +Remote requests synchronization tests + +A remote command must be done when its XML-RPC call returns, or report an error, +including when it is sent as soon as DataLab accepts connections, i.e. while the +main window may still be set up (see DataLab Issue #398). +""" + +# guitest: show + +from __future__ import annotations + +import tempfile +import threading +import xmlrpc.client +from pathlib import Path +from typing import TYPE_CHECKING + +import numpy as np +import pytest +from sigima.objects import create_signal + +from datalab.tests import close_datalab_background, datalab_in_background_context + +if TYPE_CHECKING: + from datalab.control.proxy import RemoteProxy + + +@pytest.mark.parametrize("mode", ["files", "directory"]) +def test_remote_load_at_startup(tmp_path: Path, mode: str) -> None: + """Files loaded remotely right after connecting are loaded when the call returns + + Args: + tmp_path: Temporary directory + mode: "files" to call ``load_from_files``, "directory" to call + ``load_from_directory`` + """ + filename = tmp_path / "image.npy" + np.save(filename, np.arange(6, dtype=np.uint16).reshape(2, 3)) + with datalab_in_background_context() as proxy: + proxy.set_current_panel("image") + if mode == "files": + proxy.load_from_files([str(filename)]) + else: + proxy.load_from_directory(str(tmp_path)) + assert len(proxy.get_object_uuids("image")) == 1 + + +def test_remote_command_error() -> None: + """Errors raised by remote commands are reported to the client""" + with datalab_in_background_context() as proxy: + with pytest.raises(xmlrpc.client.Fault, match="Unknown computation function"): + proxy.calc("unknown_function") + unknown = create_signal("Unknown signal", np.arange(3.0), np.zeros(3)) + with pytest.raises(xmlrpc.client.Fault, match="KeyError"): + proxy.set_object(unknown) + # The error must not be reported again by the next command + proxy.set_current_panel("signal") + assert proxy.get_current_panel() == "signal" + + +def test_remote_command_result() -> None: + """The result of a remote command is not returned by the next command""" + with datalab_in_background_context() as proxy: + assert proxy.call_method("get_current_panel") == "signal" + # No object: the result of the previous command must not be returned instead + assert proxy.get_current_object_uuid() is None + + +def test_remote_close_during_call() -> None: + """Closing DataLab while a remote call is pending does not hang DataLab""" + errors: list[Exception] = [] + + def close_datalab(proxy: RemoteProxy) -> None: + """Close DataLab with a remote call waiting for DataLab to be ready""" + try: + proxy.call_method("close") + except Exception as exc: # pylint: disable=broad-except + errors.append(exc) + + with datalab_in_background_context() as proxy: + thread = threading.Thread(target=close_datalab, args=(proxy,), daemon=True) + thread.start() + thread.join(20.0) + hung = thread.is_alive() + if hung: + # Requesting a hung DataLab to close would hang the test as well + close_datalab_background(request_close=False) + assert not hung, "DataLab hung while closing during remote call" + assert not errors + + +if __name__ == "__main__": + with tempfile.TemporaryDirectory() as tmpdir: + for load_mode in ("files", "directory"): + test_remote_load_at_startup(Path(tmpdir), load_mode) + test_remote_command_error() + test_remote_command_result() + test_remote_close_during_call() diff --git a/doc/release_notes/release_1.04.md b/doc/release_notes/release_1.04.md index 718d4b8e4..6c627077c 100644 --- a/doc/release_notes/release_1.04.md +++ b/doc/release_notes/release_1.04.md @@ -167,3 +167,10 @@ DataLab now uses stable UUID-based object references across object trees, result **Signal creation:** * The blackbody signal (**Create > Blackbody (Planck's law)**) now has axis titles and units (wavelength in m, spectral radiance in W·sr⁻¹·m⁻³), which were missing (fixes [Issue #400](https://github.com/DataLab-Platform/DataLab/issues/400)) + +**Remote control:** + +* Fixed files loaded remotely with `load_from_files` or `load_from_directory` being sometimes silently ignored, or still loading after the call had returned, when requested right after DataLab started: requests received while DataLab is starting are now processed as soon as its main window is ready (fixes [Issue #398](https://github.com/DataLab-Platform/DataLab/issues/398)) +* Errors raised by remote commands, such as `calc` with an unknown computation function or `set_object` with an unknown object, are now reported to the client instead of being silently ignored +* Fixed remote commands sometimes returning the result of the previous command instead of their own: for example, `get_current_object_uuid` could return a panel name instead of `None` when there was no current object +* Fixed DataLab freezing when closed while a remote command was still running (e.g. when closing the main window with `call_method("close")`)