Save method list via connection check to XMLRPC server.

[fujitatomoya]

address part of https://github.com/ros2/ros2cli/issues/795, significant performance improvement for ros2 daemon.

Signed-off-by: Tomoya Fujita Tomoya.Fujita@sony.com

[fujitatomoya]

@audrow requesting review on this.

[iuhilnehc-ynos]

This patch will make applications using NodeStrategy with no_daemon=False use direct_node instead of daemon_node, that's not what we expected.

1. the DaemonNode.connected is called in is_daemon_running while NodeStrategy initialization
2. but a new DaemonNode will be created after is_daemon_running https://github.com/ros2/ros2cli/blob/2a3e100a951757d354d519d7c84669af45b673d9/ros2cli/ros2cli/node/strategy.py#L27-L28 so the _method in self._daemon_node instance will be empty.
3. when the NodeStrategy call __getattr__, it will use the direct_node property finally (which looks like a fallback mechanism) https://github.com/ros2/ros2cli/blob/2a3e100a951757d354d519d7c84669af45b673d9/ros2cli/ros2cli/node/strategy.py#L58-L61

I think the DaemonNode._methods can be initialized in DaemonNode.__init__, and the DaemonNode should be only created once in the NodeStrategy.__init__.

[fujitatomoya]

I think the DaemonNode._methods can be initialized in DaemonNode.init, and the DaemonNode should be only created once in the NodeStrategy.init.

if we want to initialize method list in DaemonNode constructor, when the daemon process is not running it will generate the exception. that is fine but we need to add try & except statement some places. How about keep the current implementation and call connected method before RPC call? i think that is common behavior since daemon process could be running over network.

[fujitatomoya]

diff --git a/ros2cli/ros2cli/node/daemon.py b/ros2cli/ros2cli/node/daemon.py
index 67989b1..a130348 100644
--- a/ros2cli/ros2cli/node/daemon.py
+++ b/ros2cli/ros2cli/node/daemon.py
@@ -34,16 +34,16 @@ class DaemonNode:
         self._proxy = ServerProxy(
             daemon.get_xmlrpc_server_url(),
             allow_none=True)
-        self._methods = []
+        self._methods = [
+            method
+            for method in self._proxy.system.listMethods()
+            if not method.startswith('system.')
+        ]

     @property
     def connected(self):
         try:
-            self._methods = [
-                method
-                for method in self._proxy.system.listMethods()
-                if not method.startswith('system.')
-            ]
+            self._proxy.system.listMethods()
         except ConnectionRefusedError:
             return False
         return True
@@ -69,8 +69,11 @@ def is_daemon_running(args):

     :param args: `DaemonNode` arguments namespace.
     """
-    with DaemonNode(args) as node:
-        return node.connected
+    try:
+        with DaemonNode(args) as node:
+            return node.connected
+    except Exception:
+        return False

 def shutdown_daemon(args, timeout=None):
@@ -85,18 +88,22 @@ def shutdown_daemon(args, timeout=None):
       `False` if it was already shut down.
     :raises: if it fails to shutdown the daemon.
     """
-    with DaemonNode(args) as node:
-        if not node.connected:
-            return False
-        node.system.shutdown()
-        if timeout is not None:
-            predicate = (lambda: not node.connected)
-            if not wait_for(predicate, timeout):
-                raise RuntimeError(
-                    'Timed out waiting for '
-                    'daemon to shutdown'
-                )
-        return True
+    try:
+        with DaemonNode(args) as node:
+            if not node.connected:
+                return False
+            node.system.shutdown()
+            if timeout is not None:
+                predicate = (lambda: not node.connected)
+                if not wait_for(predicate, timeout):
+                    raise RuntimeError(
+                      'Timed out waiting for '
+                      'daemon to shutdown'
+                    )
+            return True
+    except Exception as ex:
+        raise RuntimeError("Failed to shutdown daemon node, %s" % ex)
+

 def spawn_daemon(args, timeout=None, debug=False):
diff --git a/ros2cli/ros2cli/node/strategy.py b/ros2cli/ros2cli/node/strategy.py
index f0a4ea8..3041113 100644
--- a/ros2cli/ros2cli/node/strategy.py
+++ b/ros2cli/ros2cli/node/strategy.py
@@ -14,7 +14,6 @@

 from ros2cli.node.daemon import add_arguments as add_daemon_node_arguments
 from ros2cli.node.daemon import DaemonNode
-from ros2cli.node.daemon import is_daemon_running
 from ros2cli.node.daemon import spawn_daemon
 from ros2cli.node.direct import add_arguments as add_direct_node_arguments
 from ros2cli.node.direct import DirectNode
@@ -24,12 +23,15 @@ class NodeStrategy:

     def __init__(self, args):
         use_daemon = not getattr(args, 'no_daemon', False)
-        if use_daemon and is_daemon_running(args):
-            self._daemon_node = DaemonNode(args)
-            self._direct_node = None
-        else:
-            if use_daemon:
+        if use_daemon:
+            try:
+                self._daemon_node = DaemonNode(args)
+                self._direct_node = None
+            except Exception:
                 spawn_daemon(args)
+                self._direct_node = DirectNode(args)
+                self._daemon_node = None
+        else:
             self._direct_node = DirectNode(args)
             self._daemon_node = None
         self._args = args
diff --git a/ros2cli/ros2cli/verb/daemon/stop.py b/ros2cli/ros2cli/verb/daemon/stop.py
index 0faa548..dd7a916 100644
--- a/ros2cli/ros2cli/verb/daemon/stop.py
+++ b/ros2cli/ros2cli/verb/daemon/stop.py
@@ -20,7 +20,10 @@ class StopVerb(VerbExtension):
     """Stop the daemon if it is running."""

     def main(self, *, args):
-        if shutdown_daemon(args, timeout=10.0):
-            print('The daemon has been stopped')
-        else:
-            print('The daemon is not running')
+        try:
+            if shutdown_daemon(args, timeout=10.0):
+                print('The daemon has been stopped')
+            else:
+                print('The daemon is not running')
+        except Exception as ex:
+            print("The daemon is not running, %s" % ex)
\ No newline at end of file
diff --git a/ros2cli/test/test_strategy.py b/ros2cli/test/test_strategy.py
index 7281081..ee468ad 100644
--- a/ros2cli/test/test_strategy.py
+++ b/ros2cli/test/test_strategy.py
@@ -51,6 +51,16 @@ def test_with_no_daemon_running(enforce_no_daemon_is_running):
         assert node._direct_node is not None

+def test_with_daemon_spawn(enforce_no_daemon_is_running):
+    with NodeStrategy(args=[]) as node:
+        assert node._daemon_node is None
+        assert node._direct_node is not None
+    # Daemon should be spawned by NodeStrategy call above
+    with NodeStrategy(args=[]) as node:
+        assert node._daemon_node is not None
+        assert node._direct_node is None
+
+
 def test_enforce_no_daemon(enforce_daemon_is_running):
     args = argparse.Namespace(no_daemon=True)
     with NodeStrategy(args=args) as node:

[fujitatomoya]

How about keep the current implementation and call connected method before RPC call? i think that is common behavior since daemon process could be running over network.

see https://github.com/ros2/ros2cli/pull/796/commits/d99696878a09750d35c8c7a96d2647dfdd89c99f, considering backport, this would be better?

[fujitatomoya]

https://build.ros2.org/job/Rpr__ros2cli__ubuntu_jammy_amd64/194/#showFailuresLink cannot be reproduced on my local test.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

Build failing with mcap related packages, https://ci.ros2.org/job/ci_linux/18002/console. I will retry the CI.

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

@Mergifyio backport humble

[mergify[bot]]

backport humble

✅ Backports have been created

* [#798 Save method list via connection check to XMLRPC server. (backport #796)](https://github.com/ros2/ros2cli/pull/798) has been created for branch `humble`