From dcb2637d718a63a500ba2f0673434648bce581d5 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 10 Aug 2020 16:09:31 -0700 Subject: [PATCH 1/2] Do not use event handler for loading composable nodes 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 https://github.com/ros2/launch_ros/pull/16, and doesn't appear to be necessary. Signed-off-by: Jacob Perron --- .../actions/composable_node_container.py | 13 +-- .../actions/test_composable_node_container.py | 104 ++++++++++++++++++ 2 files changed, 107 insertions(+), 10 deletions(-) create mode 100644 test_launch_ros/test/test_launch_ros/actions/test_composable_node_container.py diff --git a/launch_ros/launch_ros/actions/composable_node_container.py b/launch_ros/launch_ros/actions/composable_node_container.py index d96cedd80..e194d8472 100644 --- a/launch_ros/launch_ros/actions/composable_node_container.py +++ b/launch_ros/launch_ros/actions/composable_node_container.py @@ -92,16 +92,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]] diff --git a/test_launch_ros/test/test_launch_ros/actions/test_composable_node_container.py b/test_launch_ros/test/test_launch_ros/actions/test_composable_node_container.py new file mode 100644 index 000000000..a63587d2a --- /dev/null +++ b/test_launch_ros/test/test_launch_ros/actions/test_composable_node_container.py @@ -0,0 +1,104 @@ +# 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 +from launch_ros.utilities import get_node_name_count + +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, + ) + ], + ), + ] + + context = _assert_launch_no_errors(actions) + assert get_node_name_count(context, f'/{TEST_CONTAINER_NAMESPACE}/{TEST_CONTAINER_NAME}') == 1 + assert get_node_name_count(context, f'/{TEST_NODE_NAMESPACE}/{TEST_NODE_NAME}') == 1 + + +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), + ] + + context = _assert_launch_no_errors(actions) + assert get_node_name_count(context, f'/{TEST_CONTAINER_NAMESPACE}/{TEST_CONTAINER_NAME}') == 1 + assert get_node_name_count(context, f'/{TEST_NODE_NAMESPACE}/{TEST_NODE_NAME}') == 1 From 25996bb1da7d476f42cf1f251a65212c4be0d867 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Wed, 12 Aug 2020 11:45:05 -0700 Subject: [PATCH 2/2] Fix lint errors Signed-off-by: Jacob Perron --- launch_ros/launch_ros/actions/composable_node_container.py | 2 -- .../test_launch_ros/actions/test_composable_node_container.py | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/launch_ros/launch_ros/actions/composable_node_container.py b/launch_ros/launch_ros/actions/composable_node_container.py index e194d8472..903f29f0f 100644 --- a/launch_ros/launch_ros/actions/composable_node_container.py +++ b/launch_ros/launch_ros/actions/composable_node_container.py @@ -19,8 +19,6 @@ import warnings 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 diff --git a/test_launch_ros/test/test_launch_ros/actions/test_composable_node_container.py b/test_launch_ros/test/test_launch_ros/actions/test_composable_node_container.py index a63587d2a..8b73a53b6 100644 --- a/test_launch_ros/test/test_launch_ros/actions/test_composable_node_container.py +++ b/test_launch_ros/test/test_launch_ros/actions/test_composable_node_container.py @@ -74,7 +74,7 @@ def test_composable_node_container(): def test_composable_node_container_in_group_with_launch_configuration_in_description(): """ - Test launch configuration is passed to ComposableNode description inside GroupAction + Test launch configuration is passed to ComposableNode description inside GroupAction. This is a regression test for #114. """