Fix remote file loading intermittently ignored at DataLab startup

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

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,

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()

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")`)