Do not use event handler for loading composable nodes (#170)

Fixes #114.

Due to the asynchronous nature of the LoadComposableNodes action, an event handler causes the launch configuration is popped if ComposableNodeContainer appears inside a group or include action.
It seems to me we can simply return the load node action, which will get executed after the ComposableNodeContainer action. The use of an event handler is vestigial of a refactoring done in #16, and doesn't appear to be necessary.

Signed-off-by: Jacob Perron <[email protected]>

Author: jacobperron

launch_ros/launch_ros/actions/composable_node_container.py

@@ -18,8 +18,6 @@
 from typing import Optional
 
 from launch.action import Action
-from launch.actions import RegisterEventHandler
-from launch.event_handlers.on_process_start import OnProcessStart
 from launch.launch_context import LaunchContext
 from launch.some_substitutions_type import SomeSubstitutionsType
 
@@ -65,16 +63,9 @@ def execute(self, context: LaunchContext) -> Optional[List[Action]]:
             from .load_composable_nodes import LoadComposableNodes
             # Perform load action once the container has started.
             load_actions = [
-                RegisterEventHandler(
-                    event_handler=OnProcessStart(
-                        target_action=self,
-                        on_start=[
-                            LoadComposableNodes(
-                                composable_node_descriptions=self.__composable_node_descriptions,
-                                target_container=self
-                            )
-                        ]
-                    )
+                LoadComposableNodes(
+                    composable_node_descriptions=self.__composable_node_descriptions,
+                    target_container=self
                 )
             ]
         container_actions = super().execute(context)  # type: Optional[List[Action]]

test_launch_ros/test/test_launch_ros/actions/test_composable_node_container.py

@@ -0,0 +1,99 @@
+# 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 ComposableNodeContainer Action."""
+
+import asyncio
+
+from launch import LaunchDescription
+from launch import LaunchService
+from launch.actions import DeclareLaunchArgument
+from launch.actions import GroupAction
+from launch.substitutions import LaunchConfiguration
+from launch_ros.actions import ComposableNodeContainer
+from launch_ros.descriptions import ComposableNode
+
+import osrf_pycommon.process_utils
+
+TEST_CONTAINER_NAME = 'test_component_container_node_name'
+TEST_CONTAINER_NAMESPACE = 'test_component_container_namespace'
+TEST_NODE_NAME = 'test_composable_node_name'
+TEST_NODE_NAMESPACE = 'test_composable_node_namespace'
+
+
+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 test_composable_node_container():
+    """Nominal test for launching a ComposableNodeContainer."""
+    actions = [
+        ComposableNodeContainer(
+            package='rclcpp_components',
+            executable='component_container',
+            name=TEST_CONTAINER_NAME,
+            namespace=TEST_CONTAINER_NAMESPACE,
+            composable_node_descriptions=[
+                ComposableNode(
+                    package='composition',
+                    plugin='composition::Listener',
+                    name=TEST_NODE_NAME,
+                    namespace=TEST_NODE_NAMESPACE,
+                )
+            ],
+        ),
+    ]
+
+    _assert_launch_no_errors(actions)
+
+
+def test_composable_node_container_in_group_with_launch_configuration_in_description():
+    """
+    Test launch configuration is passed to ComposableNode description inside GroupAction.
+
+    This is a regression test for #114.
+    """
+    actions = [
+        GroupAction([
+            DeclareLaunchArgument(name='test_arg', default_value='True'),
+            ComposableNodeContainer(
+                package='rclcpp_components',
+                executable='component_container',
+                name=TEST_CONTAINER_NAME,
+                namespace=TEST_CONTAINER_NAMESPACE,
+                composable_node_descriptions=[
+                    ComposableNode(
+                        package='composition',
+                        plugin='composition::Listener',
+                        name=TEST_NODE_NAME,
+                        namespace=TEST_NODE_NAMESPACE,
+                        parameters=[{'use_sim_time': LaunchConfiguration('test_arg')}],
+                    )
+                ],
+            ),
+        ], scoped=True),
+    ]
+
+    _assert_launch_no_errors(actions)

test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py

@@ -20,7 +20,6 @@
 
 from launch import LaunchDescription
 from launch import LaunchService
-from launch.actions import GroupAction
 from launch_ros.actions import LoadComposableNodes
 from launch_ros.descriptions import ComposableNode
 
@@ -101,9 +100,9 @@ def _load_composable_node(
         composable_node_descriptions=[
             ComposableNode(
                 package=package,
-                plugin=plugin,
-                name=name,
-                namespace=namespace,
+                node_plugin=plugin,
+                node_name=name,
+                node_namespace=namespace,
                 parameters=parameters,
                 remappings=remappings,
             )
@@ -119,7 +118,7 @@ def mock_component_container():
 
 def test_load_node(mock_component_container):
     """Test loading a node."""
-    context = _assert_launch_no_errors([
+    _assert_launch_no_errors([
         _load_composable_node(
             package='foo_package',
             plugin='bar_plugin',
@@ -142,7 +141,7 @@ def test_load_node(mock_component_container):
 
 def test_load_node_with_remaps(mock_component_container):
     """Test loading a node with remappings."""
-    context = _assert_launch_no_errors([
+    _assert_launch_no_errors([
         _load_composable_node(
             package='foo_package',
             plugin='bar_plugin',
@@ -171,7 +170,7 @@ def test_load_node_with_remaps(mock_component_container):
 
 def test_load_node_with_params(mock_component_container):
     """Test loading a node with parameters."""
-    context = _assert_launch_no_errors([
+    _assert_launch_no_errors([
         _load_composable_node(
             package='foo_package',
             plugin='bar_plugin',