Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load composable nodes in sequence #315

Merged
merged 2 commits into from Apr 26, 2022

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Apr 25, 2022

Fixes #312

Each _load_composable_node function creates its own LoadComposableNodes action. When there are multiple LoadComposableNodes actions it appears possible for them to load the nodes in a different order from the launch description.

I think this is happening because the actual loading is passed to asyncio's run_in_executor()' method. The docs say this uses a concurrent.futures.ThreadPoolExecutor, so the order nodes get loaded depends on the scheduling of the threads in the executor.

This PR fixes the test by loading both composable nodes in the same LoadComposableNodes action. This works because it explicitly loads the composable nodes in sequence.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz added the bug Something isn't working label Apr 25, 2022
@sloretz sloretz self-assigned this Apr 25, 2022
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz added this to In progress in Humble Hawksbill via automation Apr 25, 2022
@sloretz sloretz moved this from In progress to In review in Humble Hawksbill Apr 25, 2022
@sloretz
Copy link
Contributor Author

sloretz commented Apr 25, 2022

CI (build: --packages-up-to test_launch_ros test: --packages-select test_launch_ros)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

The other alternatives is to test that mock_component_container.requests contains both expected requests, regardless of order.
Though this seems fine to me.

@ivanpauno
Copy link
Member

Rpr failures seem unrelated, @sloretz could you double check them?

@sloretz
Copy link
Contributor Author

sloretz commented Apr 26, 2022

Rpr failures seem unrelated, @sloretz could you double check them?

The same 3 test failures happened in another PR job too, so they're probably unrelated.

https://build.ros2.org/job/Rpr__launch_ros__ubuntu_jammy_amd64/24/#showFailuresLink

@sloretz sloretz merged commit 7fa69bc into master Apr 26, 2022
Humble Hawksbill automation moved this from In review to Done Apr 26, 2022
@delete-merged-branch delete-merged-branch bot deleted the sloretz__launch_ros__out_of_order_loading branch April 26, 2022 15:38
@clalancette
Copy link
Contributor

@sloretz Should we backport this to the humble branch as well?

@sloretz
Copy link
Contributor Author

sloretz commented Apr 26, 2022

@Mergifyio backport humble

mergify bot pushed a commit that referenced this pull request Apr 26, 2022
* Load composable nodes in sequence

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Undo whitespace change

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
(cherry picked from commit 7fa69bc)
@mergify
Copy link

mergify bot commented Apr 26, 2022

backport humble

✅ Backports have been created

Keng12 pushed a commit to Keng12/launch_ros that referenced this pull request Apr 27, 2022
* Load composable nodes in sequence

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Undo whitespace change

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Keng12 <k.chaig1@gmail.com>
sloretz added a commit that referenced this pull request Apr 28, 2022
* Load composable nodes in sequence

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Undo whitespace change

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
(cherry picked from commit 7fa69bc)

Co-authored-by: Shane Loretz <sloretz@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

test_load_composable_nodes.py flaky on Windows
3 participants