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.

Sign up for GitHub

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

Jump to bottom

Avoid losing waitable handles while using MultiThreadedExecutor #2109

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Avoid losing waitable handles while using MultiThreadedExecutor #2109

merged 4 commits into from
Mar 13, 2023

Conversation

Barry-Xu-2018
Copy link
Collaborator

Address #2012

This patch may not be the best fix.
But I hope to find the best fix by discussing this patch.

@Barry-Xu-2018 Barry-Xu-2018 marked this pull request as draft February 21, 2023 09:33
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-fix-bug-rclcpp-2012 branch from 7ac0f83 to aeaf0f4 Compare February 22, 2023 06:46
@Barry-Xu-2018 Barry-Xu-2018 marked this pull request as ready for review February 22, 2023 06:47
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-fix-bug-rclcpp-2012 branch from 324f864 to 3eaf60a Compare February 22, 2023 09:28
iuhilnehc-ynos
iuhilnehc-ynos approved these changes Feb 23, 2023
View reviewed changes
Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

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

Personally, I can't figure out a better way to fix the issue.
So it LGTM.

rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp Outdated Show resolved Hide resolved
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-fix-bug-rclcpp-2012 branch from e00bfd2 to 09262fb Compare February 23, 2023 05:06
@fujitatomoya fujitatomoya mentioned this pull request Feb 28, 2023
Closed
fujitatomoya
fujitatomoya approved these changes Feb 28, 2023
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@fujitatomoya
Copy link
Collaborator

This changes ABI, so that we cannot backport this to humble. but i believe this is the problem must be addressed to Iron Release. I will start the CI.

@clalancette @wjwwood requesting another review on this.

@fujitatomoya
Copy link
Collaborator

CI:

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

@fujitatomoya
Copy link
Collaborator

@Barry-Xu-2018 can you rebase this? we are having build failure, see https://ci.ros2.org/job/ci_linux/18122/console

@Barry-Xu-2018
Avoid losing waitable handles while using MultiThreadedExecutor
ab50018 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Remove wrong change
122e180 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Optimize codes
9f25339 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-fix-bug-rclcpp-2012 branch from 09262fb to 9f25339 Compare March 1, 2023 05:25
@Barry-Xu-2018
Copy link
Collaborator Author

Rpr__rclcpp__ubuntu_jammy_amd64 failed since latest code use service_introspection head file which isn't included in build system.

@Barry-Xu-2018
Copy link
Collaborator Author

@fujitatomoya Rebase was done.

@Barry-Xu-2018
Update test case 'get_next_waitable' and remove trick code for this t…
348ab51 
…est case

Signed-off-by: Barry Xu <barry.xu@sony.com>
@fujitatomoya
Copy link
Collaborator

fujitatomoya commented Mar 1, 2023 •
edited by clalancette
Loading

CI:

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

@Barry-Xu-2018
Copy link
Collaborator Author

After checking,
Linux failure is related to python_qt_binding. So it isn't related to this patch.
Linux-aarch64 failure is related to internal error for CI system.

@clalancette
Copy link
Contributor

Linux-aarch64 failure is related to internal error for CI system.

Yeah, it got re-run. I've updated Tomoya's post to show the re-run.

While I don't think this PR is causing the CI to go yellow, I'm going to re-run this anyway:

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

@fujitatomoya
Copy link
Collaborator

@clalancette @mjcarroll thanks for checking on this.

https://ci.ros2.org/job/ci_windows/18812/testReport/junit/launch_testing_examples.launch_testing_examples/check_multiple_nodes_launch_test/launch_testing_examples_check_multiple_nodes_launch_test/ is not related, i will go ahead to merge this.

@fujitatomoya fujitatomoya merged commit 232262c into ros2:rolling Mar 13, 2023
@fujitatomoya
Copy link
Collaborator

this cannot be backport to humble since it requires ABI change.

alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 28, 2023
@Barry-Xu-2018
Avoid losing waitable handles while using MultiThreadedExecutor (ros2…
c3052ca 
…#2109)

Signed-off-by: Barry Xu <barry.xu@sony.com>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023
@Barry-Xu-2018
Avoid losing waitable handles while using MultiThreadedExecutor (ros2…
d41b913 
…#2109)

Signed-off-by: Barry Xu <barry.xu@sony.com>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023
@Barry-Xu-2018
Avoid losing waitable handles while using MultiThreadedExecutor (ros2…
c771dbd 
…#2109)

Signed-off-by: Barry Xu <barry.xu@sony.com>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023
@Barry-Xu-2018
Avoid losing waitable handles while using MultiThreadedExecutor (ros2…
c71ba0d 
…#2109)

Signed-off-by: Barry Xu <barry.xu@sony.com>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request May 3, 2023
@Barry-Xu-2018
Avoid losing waitable handles while using MultiThreadedExecutor (ros2…
3953908 
…#2109)

Signed-off-by: Barry Xu <barry.xu@sony.com>
@jmachowinski jmachowinski mentioned this pull request Nov 13, 2023
Closed
jmachowinski pushed a commit to jmachowinski/rclcpp that referenced this pull request Dec 6, 2023
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
7208854 
…or (ros2#2109)"

This reverts commit 232262c.
jmachowinski pushed a commit to cellumation/rclcpp that referenced this pull request Dec 6, 2023
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
898bb90 
…or (ros2#2109)"

This reverts commit 232262c.
jmachowinski pushed a commit to jmachowinski/rclcpp that referenced this pull request Dec 8, 2023
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
fbadea8 
…or (ros2#2109)"

This reverts commit 232262c.
jmachowinski pushed a commit to jmachowinski/rclcpp that referenced this pull request Dec 14, 2023
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
3912404 
…or (ros2#2109)"

This reverts commit 232262c.
jmachowinski pushed a commit to jmachowinski/rclcpp that referenced this pull request Dec 14, 2023
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
9f2ec0c 
…or (ros2#2109)"

This reverts commit 232262c.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
This was referenced Jan 4, 2024
Closed
Merged
@wjwwood
Copy link
Member

wjwwood commented Jan 4, 2024

Just to help with cross-referencing, we're discussing in a few places whether or not this is the right fix for this issue: #2371 (comment)

I'm currently leaning towards the idea that this is not the right fix and instead we should change specific waitables to address this issue.

Barry-Xu-2018 added a commit to Barry-Xu-2018/rclcpp that referenced this pull request Jan 12, 2024
@Barry-Xu-2018
Avoid losing waitable handles while using MultiThreadedExecutor (ros2…
320c111 
…#2109)

Signed-off-by: Barry Xu <barry.xu@sony.com>
jmachowinski pushed a commit to jmachowinski/rclcpp that referenced this pull request Jan 29, 2024
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
522906b 
…or (ros2#2109)"

This reverts commit 232262c.
jmachowinski pushed a commit to jmachowinski/rclcpp that referenced this pull request Jan 29, 2024
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
6c4c6a2 
…or (ros2#2109)"

This reverts commit 232262c.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
wjwwood added a commit that referenced this pull request Jan 30, 2024
@wjwwood
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
e8c7270 
…or (#2109)"

This reverts commit 232262c.
jmachowinski pushed a commit to cellumation/rclcpp that referenced this pull request Mar 26, 2024
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
69add7e 
…or (ros2#2109)"

This reverts commit 232262c.
wjwwood added a commit that referenced this pull request Mar 27, 2024
@wjwwood
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
633898f 
…or (#2109)"

This reverts commit 232262c.
wjwwood added a commit that referenced this pull request Mar 27, 2024
@wjwwood
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
c70e997 
…or (#2109)"

This reverts commit 232262c.
wjwwood added a commit that referenced this pull request Mar 27, 2024
@wjwwood
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
d7601b5 
…or (#2109)"

This reverts commit 232262c.
jmachowinski pushed a commit to cellumation/rclcpp that referenced this pull request Apr 2, 2024
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
710b41f 
…or (ros2#2109)"

This reverts commit 232262c.
wjwwood added a commit that referenced this pull request Apr 3, 2024
@wjwwood
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
60a7926 
…or (#2109)"

This reverts commit 232262c.
wjwwood added a commit that referenced this pull request Apr 4, 2024
@wjwwood
Revert "Avoid losing waitable handles while using MultiThreadedExecut…
8a470c3 
…or (#2109)"

This reverts commit 232262c.

Signed-off-by: William Woodall <william@osrfoundation.org>
@jmachowinski jmachowinski mentioned this pull request May 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iuhilnehc-ynos iuhilnehc-ynos iuhilnehc-ynos approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Barry-Xu-2018 @fujitatomoya @clalancette @wjwwood @mjcarroll @iuhilnehc-ynos