Fix race with launch context changes when loading composable nodes #166

jacobperron · 2020-08-04T23:12:52Z

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.

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 <jacob@openrobotics.org>

ivanpauno

Can you add a regression test?

ivanpauno · 2020-08-05T16:49:48Z

@jacobperron Does this fix #114?

hidmic

Thanks for the fix and +1 to having a regression test.

jacobperron · 2020-08-05T16:55:52Z

Can you add a regression test?

Will do.

Does this fix #114?

I had completely forgotten about that issue. I'll check to see if it fixes it.

jacobperron · 2020-08-05T18:37:36Z

So, it appears this PR fixes the example mentioned in #114 (comment), which involves setting the namespace, but not the original issue that involves setting parameters on the node. I'll look into if a similar fix to the one proposed in this PR can be used to fix #114.

ivanpauno · 2020-08-05T18:41:16Z

So, it appears this PR fixes the example mentioned in #114 (comment), which involves setting the namespace, but not the original issue that involves setting parameters on the node. I'll look into if a similar fix to the one proposed in this PR can be used to fix #114.

That's weird, because get_composable_node_load_request also resolves the parameters to be loaded.

jacobperron · 2020-08-05T18:41:52Z

That's weird, because get_composable_node_load_request also resolves the parameters to be loaded.

Is there a difference between "global" params (aka launch config ros_params) and parameters passed directly to a node?

jacobperron · 2020-08-05T18:44:14Z

hmm, but it's not the parameters that are being dropped, rather a user-defined launch configuration "bar_arg". I can do some more debugging.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2020-08-07T01:17:30Z

I've added some regression tests in 719ced8.

There's still a bug related to parameters (and some other corner-cases); I'll open up separate PRs for those.

test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py

ivanpauno · 2020-08-07T13:19:05Z

test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py

+    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


maybe the sent request could be a property, so we don't have to check a private member variable

I followed a similar pattern that is used in test_node.py.

I think a user should not have access to that information; it seems like an implementation detail subject to change.

I followed a similar pattern that is used in test_node.py.

Yeah, it's not a great pattern, but acceptable I guess.

I think a user should have access to that information; it seems like an implementation detail subject to change.

That sounds contradictory, I guess you meant "should not".

I prefer not testing implementation details, but I don't feel strongly in this case.

Yeah, sorry, I meant "should not" (updated my comment).

I guess the ideal way to write tests in this situation would be to write a mock component container and assert things about it (e.g. if the correct service request was received). It adds a bit more to the test complexity, but could be better long-term. What do you think?

What do you think?

Sounds like a good proposal to me, though I don't mind approving this as-is.

I went ahead and refactored the tests to use a mock object. I think it's much nicer. PTAL 5d9dcf3

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

ivanpauno

LGTM

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

This was only needed for the tests before the refactor. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2020-08-10T19:22:19Z

test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py

+    assert request.parameters[0].value.type == ParameterType.PARAMETER_STRING
+    assert request.parameters[0].value.string_value == 'test_value_param1'
+    assert request.parameters[1].name == 'test.param2'
+    # TODO(jacobperron): I would expect this to be a double value, but we can only pass strings


I'll take a look at resolving this in a follow-up.

jacobperron · 2020-08-10T21:30:53Z

ugh, there's an issue with the tests where they hang sometimes... I can reproduce locally. I think there's an issue shutting down the launch context before LoadComposableNodes has had a chance to make a service request.

jacobperron · 2020-08-11T02:38:09Z

I've discovered a race in launch (reported in #169), with a proposed fix in ros2/launch#449. With the proposed fix, the tests in the PR pass, otherwise we can expect them to hang forever.

There's no need to run asynchronously and manually send a shutdown signal. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

ivanpauno · 2020-08-11T13:27:25Z

I've discovered a race in launch (reported in #169), with a proposed fix in ros2/launch#449. With the proposed fix, the tests in the PR pass, otherwise we can expect them to hang forever.

I understand the race, but what is causing it here?
The thread you're creating isn't emiting any event, so I would like to understand what exactly is causing the race here (the launch service is running in a single threaded event loop)

wjwwood

Makes sense to me for getting the right values when the action is executed.

jacobperron · 2020-08-14T18:29:55Z

Sadly, it still seems like there's a race somewhere causing the tests to hang somteimes 😞

Also added some debug logs to the load node action for posterity. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2020-08-14T19:43:57Z

I wasn't properly shutting down the mock container and so the synchronous service call in LoadComposableNodes was blocking indefinitely. I've fixed the test in 9c50db2 and opened up a follow-up ticket to improve LoadComposableNodes such that we avoid this situation #171.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2020-08-14T19:49:52Z

Note, I don't expect the PR CI job to pass until ros2/launch#449 is released.

jacobperron · 2020-08-14T19:50:52Z

Linux
Linux-aarch64
macOS
Windows (unrelated cppcheck failure occurring in latest nightly)

ivanpauno

LGTM!
I didn't realize that shutdown wasn't done properly

test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py

jacobperron · 2020-08-17T16:53:32Z

@ros-pull-request-builder retest this please

) * 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 <jacob@openrobotics.org> * Add regression tests for LoadComposableNode Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Properly shutdown mock conatiner node Also added some debug logs to the load node action for posterity. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

…odes (#166) (#206) * 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 <jacob@openrobotics.org> * Add regression tests for LoadComposableNode Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Properly shutdown mock conatiner node Also added some debug logs to the load node action for posterity. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix docblock in LoadComposableNodes (#207) Signed-off-by: Jacob Perron <jacob@openrobotics.org>

) * 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 <jacob@openrobotics.org> * Add regression tests for LoadComposableNode Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Properly shutdown mock conatiner node Also added some debug logs to the load node action for posterity. Backporting note: As far as I can tell, the *Composable* launch_ros classes in Dashing are not setup to be able to be tested. We would need to add a bunch of additional dependencies to write tests here. I didn't think that was worth it for Dashing, so I just removed the tests. Signed-off-by: Jacob Perron <jacob@openrobotics.org> Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

) (#238) * 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 <jacob@openrobotics.org> * Add regression tests for LoadComposableNode Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Properly shutdown mock conatiner node Also added some debug logs to the load node action for posterity. Backporting note: As far as I can tell, the *Composable* launch_ros classes in Dashing are not setup to be able to be tested. We would need to add a bunch of additional dependencies to write tests here. I didn't think that was worth it for Dashing, so I just removed the tests. Signed-off-by: Jacob Perron <jacob@openrobotics.org> Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

jacobperron added the bug Something isn't working label Aug 4, 2020

jacobperron requested review from hidmic and ivanpauno August 4, 2020 23:12

jacobperron self-assigned this Aug 4, 2020

ivanpauno approved these changes Aug 5, 2020

View reviewed changes

hidmic approved these changes Aug 5, 2020

View reviewed changes

Add regression tests for LoadComposableNode

719ced8

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

ivanpauno reviewed Aug 7, 2020

View reviewed changes

Parameterize target container in text fixture

a12906b

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

ivanpauno approved these changes Aug 7, 2020

View reviewed changes

jacobperron added 2 commits August 10, 2020 12:15

Refactor tests to use mock component container

5d9dcf3

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Don't store requests

074bdac

This was only needed for the tests before the refactor. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron commented Aug 10, 2020

View reviewed changes

Launch actions synchronously

d57bc8d

There's no need to run asynchronously and manually send a shutdown signal. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron force-pushed the jacob/fix_load_composable_node_remap branch from 782776c to d57bc8d Compare August 11, 2020 02:44

jacobperron requested a review from ivanpauno August 11, 2020 02:45

jacobperron mentioned this pull request Aug 11, 2020

Do not use event handler for loading composable nodes #170

Merged

wjwwood approved these changes Aug 11, 2020

View reviewed changes

jacobperron added this to Proposed in Foxy Patch Release 3 via automation Aug 12, 2020

Properly shutdown mock conatiner node

9c50db2

Also added some debug logs to the load node action for posterity. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Fix lint

cf1437a

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

ivanpauno approved these changes Aug 14, 2020

View reviewed changes

test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py Outdated Show resolved Hide resolved

jacobperron merged commit e181799 into master Aug 17, 2020

jacobperron deleted the jacob/fix_load_composable_node_remap branch August 17, 2020 16:59

jacobperron removed this from Needs backport in Foxy Patch Release 3 Oct 9, 2020

jacobperron added this to Proposed in Foxy Patch Release 4 via automation Oct 9, 2020

jacobperron moved this from Proposed to Needs backport in Foxy Patch Release 4 Oct 9, 2020

nuclearsandwich added this to Proposed in Dashing Patch Release 8 Nov 17, 2020

jacobperron mentioned this pull request Nov 20, 2020

[foxy] Fix race with launch context changes when loading composable nodes (#166) #206

Merged

jacobperron removed this from Needs backport in Foxy Patch Release 4 Nov 20, 2020

jacobperron mentioned this pull request Nov 20, 2020

Fix docblock in LoadComposableNodes #207

Merged

jacobperron removed this from Proposed in Dashing Patch Release 8 Nov 21, 2020

jacobperron added this to Proposed in Dashing Patch Release 9 via automation Nov 21, 2020

clalancette moved this from Proposed to Needs Backport in Dashing Patch Release 9 Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race with launch context changes when loading composable nodes #166

Fix race with launch context changes when loading composable nodes #166

jacobperron commented Aug 4, 2020

ivanpauno left a comment

ivanpauno commented Aug 5, 2020

hidmic left a comment

jacobperron commented Aug 5, 2020

jacobperron commented Aug 5, 2020

ivanpauno commented Aug 5, 2020

jacobperron commented Aug 5, 2020

jacobperron commented Aug 5, 2020 •

edited

Loading

jacobperron commented Aug 7, 2020

ivanpauno Aug 7, 2020

jacobperron Aug 7, 2020 •

edited

Loading

ivanpauno Aug 7, 2020

jacobperron Aug 7, 2020

jacobperron Aug 7, 2020

ivanpauno Aug 7, 2020

jacobperron Aug 10, 2020

ivanpauno left a comment

jacobperron Aug 10, 2020

jacobperron commented Aug 10, 2020

jacobperron commented Aug 11, 2020

ivanpauno commented Aug 11, 2020

wjwwood left a comment

jacobperron commented Aug 14, 2020

jacobperron commented Aug 14, 2020

jacobperron commented Aug 14, 2020

jacobperron commented Aug 14, 2020 •

edited

Loading

ivanpauno left a comment •

edited

Loading

jacobperron commented Aug 17, 2020

Fix race with launch context changes when loading composable nodes #166

Fix race with launch context changes when loading composable nodes #166

Conversation

jacobperron commented Aug 4, 2020

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno commented Aug 5, 2020

hidmic left a comment

Choose a reason for hiding this comment

jacobperron commented Aug 5, 2020

jacobperron commented Aug 5, 2020

ivanpauno commented Aug 5, 2020

jacobperron commented Aug 5, 2020

jacobperron commented Aug 5, 2020 • edited Loading

jacobperron commented Aug 7, 2020

ivanpauno Aug 7, 2020

Choose a reason for hiding this comment

jacobperron Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

ivanpauno Aug 7, 2020

Choose a reason for hiding this comment

jacobperron Aug 7, 2020

Choose a reason for hiding this comment

jacobperron Aug 7, 2020

Choose a reason for hiding this comment

ivanpauno Aug 7, 2020

Choose a reason for hiding this comment

jacobperron Aug 10, 2020

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

jacobperron Aug 10, 2020

Choose a reason for hiding this comment

jacobperron commented Aug 10, 2020

jacobperron commented Aug 11, 2020

ivanpauno commented Aug 11, 2020

wjwwood left a comment

Choose a reason for hiding this comment

jacobperron commented Aug 14, 2020

jacobperron commented Aug 14, 2020

jacobperron commented Aug 14, 2020

jacobperron commented Aug 14, 2020 • edited Loading

ivanpauno left a comment • edited Loading

Choose a reason for hiding this comment

jacobperron commented Aug 17, 2020

jacobperron commented Aug 5, 2020 •

edited

Loading

jacobperron Aug 7, 2020 •

edited

Loading

jacobperron commented Aug 14, 2020 •

edited

Loading

ivanpauno left a comment •

edited

Loading