Fix race with launch context changes when loading composable nodes

launch_ros/launch_ros/actions/load_composable_nodes.py

@@ -79,16 +79,18 @@ def __init__(
         self.__target_container = target_container
         self.__final_target_container_name = None  # type: Optional[Text]
         self.__logger = launch.logging.get_logger(__name__)
+        # Useful for regression testing
+        self.__load_node_requests = None
 
     def _load_node(
         self,
-        composable_node_description: ComposableNode,
+        request: composition_interfaces.srv.LoadNode.Request,
         context: LaunchContext
     ) -> None:
         """
         Load node synchronously.
 
-        :param composable_node_description: description of composable node to be loaded
+        :param request: service request to load a node
         :param context: current launch context
         """
         while not self.__rclpy_load_node_client.wait_for_service(timeout_sec=1.0):
@@ -99,7 +101,6 @@ def _load_node(
                     )
                 )
                 return
-        request = get_composable_node_load_request(composable_node_description, context)
         response = self.__rclpy_load_node_client.call(request)
         node_name = response.full_node_name if response.full_node_name else request.node_name
         if response.success:
@@ -125,7 +126,7 @@ def _load_node(
 
     def _load_in_sequence(
         self,
-        composable_node_descriptions: List[ComposableNode],
+        load_node_requests: List[composition_interfaces.srv.LoadNode.Request],
         context: LaunchContext
     ) -> None:
         """
@@ -134,13 +135,13 @@ def _load_in_sequence(
         :param composable_node_descriptions: descriptions of composable nodes to be loaded
         :param context: current launch context
         """
-        next_composable_node_description = composable_node_descriptions[0]
-        composable_node_descriptions = composable_node_descriptions[1:]
-        self._load_node(next_composable_node_description, context)
-        if len(composable_node_descriptions) > 0:
+        next_load_node_request = load_node_requests[0]
+        load_node_requests = load_node_requests[1:]
+        self._load_node(next_load_node_request, context)
+        if len(load_node_requests) > 0:
             context.add_completion_future(
                 context.asyncio_loop.run_in_executor(
-                    None, self._load_in_sequence, composable_node_descriptions, context
+                    None, self._load_in_sequence, load_node_requests, context
                 )
             )
 
@@ -169,9 +170,16 @@ def execute(
             )
         )
 
+        # Generate load requests before execute() exits to avoid race with context changing
+        # due to scope change (e.g. if loading nodes from within a GroupAction).
+        self.__load_node_requests = [
+            get_composable_node_load_request(node_description, context)
+            for node_description in self.__composable_node_descriptions
+        ]
+
         context.add_completion_future(
             context.asyncio_loop.run_in_executor(
-                None, self._load_in_sequence, self.__composable_node_descriptions, context
+                None, self._load_in_sequence, self.__load_node_requests, context
             )
         )
 

test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py

@@ -0,0 +1,153 @@
+# Copyright 2020 Open Source Robotics Foundation, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Tests for the LoadComposableNodes Action."""
+
+import asyncio
+
+from launch import LaunchDescription
+from launch import LaunchService
+from launch.actions import GroupAction
+from launch_ros.actions import ComposableNodeContainer
+from launch_ros.actions import LoadComposableNodes
+from launch_ros.actions import PushRosNamespace
+from launch_ros.actions import SetRemap
+from launch_ros.descriptions import ComposableNode
+from launch_ros.utilities import get_node_name_count
+
+import osrf_pycommon.process_utils
+
+TEST_CONTAINER_NAME = 'test_load_composable_nodes_container'
+TEST_NODE_NAME = 'test_load_composable_nodes_node'
+
+
+def _assert_launch_no_errors(actions, *, timeout_sec=1):
+    ld = LaunchDescription(actions)
+    ls = LaunchService(debug=True)
+    ls.include_launch_description(ld)
+
+    loop = osrf_pycommon.process_utils.get_loop()
+    launch_task = loop.create_task(ls.run_async())
+    loop.run_until_complete(asyncio.sleep(timeout_sec))
+    if not launch_task.done():
+        loop.create_task(ls.shutdown())
+        loop.run_until_complete(launch_task)
+    assert 0 == launch_task.result()
+    return ls.context
+
+
+def _create_node_container(*, parameters=None, remappings=None, namespace=''):
+    return ComposableNodeContainer(
+        package='rclcpp_components',
+        executable='component_container',
+        name=TEST_CONTAINER_NAME,
+        namespace=namespace,
+        output='screen',
+        parameters=parameters,
+        remappings=remappings,
+    )
+
+
+def _load_composable_node(
+    *,
+    parameters=None,
+    remappings=None,
+    target_container=TEST_CONTAINER_NAME
+):
+    return LoadComposableNodes(
+        target_container=target_container,
+        composable_node_descriptions=[
+            ComposableNode(
+                package='composition',
+                plugin='composition::Listener',
+                name=TEST_NODE_NAME,
+                parameters=parameters,
+                remappings=remappings,
+            )
+        ])
+
+
+def test_load_invalid_node():
+    """Test loading an invalid node."""
+    load_composable_nodes = LoadComposableNodes(
+        target_container=TEST_CONTAINER_NAME,
+        composable_node_descriptions=[
+            ComposableNode(
+                package='nonexistent_package',
+                plugin='this_plugin_should_not_exist',
+                name=TEST_NODE_NAME,
+            )
+        ])
+    context = _assert_launch_no_errors([_create_node_container(), load_composable_nodes])
+
+    assert get_node_name_count(context, f'/{TEST_NODE_NAME}') == 0
+    assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1
+
+
+def test_load_node():
+    """Test loading a node."""
+    context = _assert_launch_no_errors([
+        _create_node_container(), _load_composable_node()
+    ])
+
+    assert get_node_name_count(context, f'/{TEST_NODE_NAME}') == 1
+    assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1
+
+
+def test_load_node_with_global_remaps_in_group():
+    """Test loading a node with global remaps scoped to a group."""
+    load_composable_nodes_action = _load_composable_node()
+    context = _assert_launch_no_errors([
+        _create_node_container(),
+        GroupAction(
+            [
+                SetRemap('chatter', 'new_topic_name'),
+                load_composable_nodes_action,
+            ],
+            scoped=True,
+        ),
+    ])
+
+    assert get_node_name_count(context, f'/{TEST_NODE_NAME}') == 1
+    assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1
+
+    # Check the remaps in load service request
+    assert len(load_composable_nodes_action._LoadComposableNodes__load_node_requests) == 1
+    request = load_composable_nodes_action._LoadComposableNodes__load_node_requests[0]
+    assert len(request.remap_rules) == 1
+    assert request.remap_rules[0] == 'chatter:=new_topic_name'
+
+
+def test_load_node_with_namespace_in_group():
+    """Test loading a node with namespace scoped to a group."""
+    namespace = '/foo'
+    load_composable_nodes_action = _load_composable_node()
+    context = _assert_launch_no_errors([
+        _create_node_container(),
+        GroupAction(
+            [
+                PushRosNamespace(namespace),
+                load_composable_nodes_action,
+            ],
+            scoped=True,
+        ),
+    ])
+
+    assert get_node_name_count(context, f'{namespace}/{TEST_NODE_NAME}') == 1
+    assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1
+
+    # Check the namespace in load service request
+    assert len(load_composable_nodes_action._LoadComposableNodes__load_node_requests) == 1
+    request = load_composable_nodes_action._LoadComposableNodes__load_node_requests[0]
+    assert request.node_namespace == f'{namespace}'