Skip to content

Commit e814e3c

Browse files
committed
Fix race with launch context changes when loading composable nodes (#166)
* Fix race with launch context changes when loading composable nodes This bug was discovered when trying load composable nodes from a GroupAction. The ROS namespace (and presumably other remaps) pushed onto the context stack was popped after the LoadComposableNodes execute() function finished. But because the loading happens asynchronously, we need to make sure we get the necessary information from the context before execute() finishes. Signed-off-by: Jacob Perron <[email protected]> * Add regression tests for LoadComposableNode Signed-off-by: Jacob Perron <[email protected]> * Properly shutdown mock conatiner node Also added some debug logs to the load node action for posterity. Signed-off-by: Jacob Perron <[email protected]>
1 parent e0bd3f5 commit e814e3c

File tree

2 files changed

+234
-11
lines changed

2 files changed

+234
-11
lines changed

launch_ros/launch_ros/actions/load_composable_nodes.py

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -69,13 +69,13 @@ def __init__(
6969

7070
def _load_node(
7171
self,
72-
composable_node_description: ComposableNode,
72+
request: composition_interfaces.srv.LoadNode.Request,
7373
context: LaunchContext
7474
) -> None:
7575
"""
7676
Load node synchronously.
7777
78-
:param composable_node_description: description of composable node to be loaded
78+
:param request: service request to load a node
7979
:param context: current launch context
8080
"""
8181
while not self.__rclpy_load_node_client.wait_for_service(timeout_sec=1.0):
@@ -86,9 +86,21 @@ def _load_node(
8686
)
8787
)
8888
return
89-
request = get_composable_node_load_request(composable_node_description, context)
89+
self.__logger.debug(
90+
"Calling the '{}' service with request '{}'".format(
91+
self.__rclpy_load_node_client.srv_name, request
92+
)
93+
)
9094
response = self.__rclpy_load_node_client.call(request)
91-
if not response.success:
95+
self.__logger.debug("Received response '{}'".format(response))
96+
node_name = response.full_node_name if response.full_node_name else request.node_name
97+
if response.success:
98+
if node_name is not None:
99+
add_node_name(context, node_name)
100+
self.__logger.info("Loaded node '{}' in container '{}'".format(
101+
response.full_node_name, self.__final_target_container_name
102+
))
103+
else:
92104
self.__logger.error(
93105
"Failed to load node '{}' of type '{}' in container '{}': {}".format(
94106
response.full_node_name if response.full_node_name else request.node_name,
@@ -101,7 +113,7 @@ def _load_node(
101113

102114
def _load_in_sequence(
103115
self,
104-
composable_node_descriptions: List[ComposableNode],
116+
load_node_requests: List[composition_interfaces.srv.LoadNode.Request],
105117
context: LaunchContext
106118
) -> None:
107119
"""
@@ -110,13 +122,13 @@ def _load_in_sequence(
110122
:param composable_node_descriptions: descriptions of composable nodes to be loaded
111123
:param context: current launch context
112124
"""
113-
next_composable_node_description = composable_node_descriptions[0]
114-
composable_node_descriptions = composable_node_descriptions[1:]
115-
self._load_node(next_composable_node_description, context)
116-
if len(composable_node_descriptions) > 0:
125+
next_load_node_request = load_node_requests[0]
126+
load_node_requests = load_node_requests[1:]
127+
self._load_node(next_load_node_request, context)
128+
if len(load_node_requests) > 0:
117129
context.add_completion_future(
118130
context.asyncio_loop.run_in_executor(
119-
None, self._load_in_sequence, composable_node_descriptions, context
131+
None, self._load_in_sequence, load_node_requests, context
120132
)
121133
)
122134

@@ -132,9 +144,16 @@ def execute(
132144
)
133145
)
134146

147+
# Generate load requests before execute() exits to avoid race with context changing
148+
# due to scope change (e.g. if loading nodes from within a GroupAction).
149+
load_node_requests = [
150+
get_composable_node_load_request(node_description, context)
151+
for node_description in self.__composable_node_descriptions
152+
]
153+
135154
context.add_completion_future(
136155
context.asyncio_loop.run_in_executor(
137-
None, self._load_in_sequence, self.__composable_node_descriptions, context
156+
None, self._load_in_sequence, load_node_requests, context
138157
)
139158
)
140159

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
# Copyright 2020 Open Source Robotics Foundation, Inc.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
15+
"""Tests for the LoadComposableNodes Action."""
16+
17+
import threading
18+
19+
from composition_interfaces.srv import LoadNode
20+
21+
from launch import LaunchDescription
22+
from launch import LaunchService
23+
from launch.actions import GroupAction
24+
from launch_ros.actions import LoadComposableNodes
25+
from launch_ros.descriptions import ComposableNode
26+
27+
import pytest
28+
29+
from rcl_interfaces.msg import ParameterType
30+
31+
import rclpy
32+
import rclpy.context
33+
import rclpy.executors
34+
import rclpy.node
35+
36+
TEST_CONTAINER_NAME = 'mock_component_container'
37+
TEST_NODE_NAME = 'test_load_composable_nodes_node'
38+
39+
40+
class MockComponentContainer(rclpy.node.Node):
41+
42+
def __init__(self):
43+
# List of LoadNode requests received
44+
self.requests = []
45+
46+
self._context = rclpy.context.Context()
47+
rclpy.init(context=self._context)
48+
49+
super().__init__(TEST_CONTAINER_NAME, context=self._context)
50+
51+
self.load_node_service = self.create_service(
52+
LoadNode,
53+
'~/_container/load_node',
54+
self.load_node_callback
55+
)
56+
57+
self._executor = rclpy.executors.SingleThreadedExecutor(context=self._context)
58+
59+
# Start spinning in a thread
60+
self._thread = threading.Thread(
61+
target=rclpy.spin,
62+
args=(self, self._executor),
63+
daemon=True
64+
)
65+
self._thread.start()
66+
67+
def load_node_callback(self, request, response):
68+
self.requests.append(request)
69+
response.success = True
70+
response.full_node_name = f'{request.node_namespace}/{request.node_name}'
71+
response.unique_id = len(self.requests)
72+
return response
73+
74+
def shutdown(self):
75+
self._executor.shutdown()
76+
self.destroy_node()
77+
rclpy.shutdown(context=self._context)
78+
self._thread.join()
79+
80+
81+
def _assert_launch_no_errors(actions):
82+
ld = LaunchDescription(actions)
83+
ls = LaunchService(debug=True)
84+
ls.include_launch_description(ld)
85+
assert 0 == ls.run()
86+
return ls.context
87+
88+
89+
def _load_composable_node(
90+
*,
91+
package,
92+
plugin,
93+
name,
94+
namespace='',
95+
parameters=None,
96+
remappings=None,
97+
target_container=f'/{TEST_CONTAINER_NAME}'
98+
):
99+
return LoadComposableNodes(
100+
target_container=target_container,
101+
composable_node_descriptions=[
102+
ComposableNode(
103+
package=package,
104+
plugin=plugin,
105+
name=name,
106+
namespace=namespace,
107+
parameters=parameters,
108+
remappings=remappings,
109+
)
110+
])
111+
112+
113+
@pytest.fixture
114+
def mock_component_container():
115+
container = MockComponentContainer()
116+
yield container
117+
container.shutdown()
118+
119+
120+
def test_load_node(mock_component_container):
121+
"""Test loading a node."""
122+
context = _assert_launch_no_errors([
123+
_load_composable_node(
124+
package='foo_package',
125+
plugin='bar_plugin',
126+
name='test_node_name',
127+
namespace='test_node_namespace'
128+
)
129+
])
130+
131+
# Check that container recieved correct request
132+
assert len(mock_component_container.requests) == 1
133+
request = mock_component_container.requests[0]
134+
assert request.package_name == 'foo_package'
135+
assert request.plugin_name == 'bar_plugin'
136+
assert request.node_name == 'test_node_name'
137+
assert request.node_namespace == '/test_node_namespace'
138+
assert len(request.remap_rules) == 0
139+
assert len(request.parameters) == 0
140+
assert len(request.extra_arguments) == 0
141+
142+
143+
def test_load_node_with_remaps(mock_component_container):
144+
"""Test loading a node with remappings."""
145+
context = _assert_launch_no_errors([
146+
_load_composable_node(
147+
package='foo_package',
148+
plugin='bar_plugin',
149+
name='test_node_name',
150+
namespace='test_node_namespace',
151+
remappings=[
152+
('test_topic1', 'test_remap_topic1'),
153+
('test/topic/two', 'test/remap_topic2')
154+
]
155+
)
156+
])
157+
158+
# Check that container recieved correct request
159+
assert len(mock_component_container.requests) == 1
160+
request = mock_component_container.requests[0]
161+
assert request.package_name == 'foo_package'
162+
assert request.plugin_name == 'bar_plugin'
163+
assert request.node_name == 'test_node_name'
164+
assert request.node_namespace == '/test_node_namespace'
165+
assert len(request.remap_rules) == 2
166+
assert request.remap_rules[0] == 'test_topic1:=test_remap_topic1'
167+
assert request.remap_rules[1] == 'test/topic/two:=test/remap_topic2'
168+
assert len(request.parameters) == 0
169+
assert len(request.extra_arguments) == 0
170+
171+
172+
def test_load_node_with_params(mock_component_container):
173+
"""Test loading a node with parameters."""
174+
context = _assert_launch_no_errors([
175+
_load_composable_node(
176+
package='foo_package',
177+
plugin='bar_plugin',
178+
name='test_node_name',
179+
namespace='test_node_namespace',
180+
parameters=[{
181+
'test_param1': 'test_value_param1',
182+
'test.param2': '42.0',
183+
}]
184+
)
185+
])
186+
187+
# Check that container recieved correct request
188+
assert len(mock_component_container.requests) == 1
189+
request = mock_component_container.requests[0]
190+
assert request.package_name == 'foo_package'
191+
assert request.plugin_name == 'bar_plugin'
192+
assert request.node_name == 'test_node_name'
193+
assert request.node_namespace == '/test_node_namespace'
194+
assert len(request.remap_rules) == 0
195+
assert len(request.parameters) == 2
196+
assert request.parameters[0].name == 'test_param1'
197+
assert request.parameters[0].value.type == ParameterType.PARAMETER_STRING
198+
assert request.parameters[0].value.string_value == 'test_value_param1'
199+
assert request.parameters[1].name == 'test.param2'
200+
# TODO(jacobperron): I would expect this to be a double value, but we can only pass strings
201+
# assert request.parameters[1].value.type == ParameterType.PARAMETER_DOUBLE
202+
# assert request.parameters[1].value.double_value == 42.0
203+
assert request.parameters[1].value.string_value == '42.0'
204+
assert len(request.extra_arguments) == 0

0 commit comments

Comments
 (0)