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

fix: Fixed race condition in action server between is_ready and take (humble) #2635

Merged
merged 1 commit into from
Oct 31, 2024
Merged

fix: Fixed race condition in action server between is_ready and take (humble) #2635

merged 1 commit into from
Oct 31, 2024

Conversation

edgarcamilocamacho
Copy link

@edgarcamilocamacho edgarcamilocamacho commented Sep 30, 2024 •
edited
Loading

Related bug: #2242. I managed to create a situation in which I can easily replicate it as many times as I wanted.

This bug was already fixed in rolling (#2495) and iron (#2531).

I just backported the fix based on the iron fix. Now I test it and works perfectly all the times I run my node.

I tried to do it as clean as possible (fortunately the iron fix didn't touch header files and all the function prototypes where the same between iron and humble).

This was referenced Sep 30, 2024
Closed
Merged
jmachowinski
jmachowinski suggested changes Oct 1, 2024
View reviewed changes
@mjcarroll
Copy link
Member

This also needs DCO sign-off.

fujitatomoya
fujitatomoya reviewed Oct 10, 2024
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.

@edgarcamilocamacho friendly ping

@edgarcamilocamacho
Copy link
Author

edgarcamilocamacho commented Oct 25, 2024 •
edited
Loading

I changed the return nullptr; lines to throw, let me know if the messages are correct.

@fujitatomoya
Copy link
Collaborator

@edgarcamilocamacho this still needs DCO signoff.

@fujitatomoya
Copy link
Collaborator

@jmachowinski all comments are addressed, could you take an another round?

@jmachowinski
Copy link
Contributor

I will have a look on Monday.
@edgarcamilocamacho fyi for the dco to succeed you need to squash the commits and in git gui (or similar tool) press the signoff button. Afterwards you need to force push your branch.

@jmachowinski
Copy link
Contributor

Looks good to me, just the DCO missing. Perhaps also add me as co author as this is a cherry pick of my patch. (I personally don't care, but don't know what the ros policy on this kind of stuff is...)

@edgarcamilocamacho edgarcamilocamacho force-pushed the fix/isready_take_race/backport_iron_2531 branch 2 times, most recently from 38c7621 to 70c99ce Compare October 28, 2024 19:11
@edgarcamilocamacho
fix: Fixed race condition in action server between is_ready and take.…
5d4e387 
… Backport from iron ros2#2531

Signed-off-by: Camilo Camacho <camilo.im93@gmail.com>
Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@edgarcamilocamacho edgarcamilocamacho force-pushed the fix/isready_take_race/backport_iron_2531 branch from 70c99ce to 5d4e387 Compare October 28, 2024 19:14
@edgarcamilocamacho
Copy link
Author

Done, sorry if you received multiple notifications, I had a problem with my company and personal emails and did multiple push while fixing it.

@jmachowinski
Copy link
Contributor

@fujitatomoya can you run the full CI ?

@ahcorde
Copy link
Contributor

ahcorde commented Oct 29, 2024

Pulls: #2635
Gist: https://gist.githubusercontent.com/ahcorde/95876fd74a3156cdb63d5811a4a0b919/raw/092e6f72f4ae138cef880177361c4e6ac8abe767/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp_action --packages-above-and-dependencies rclcpp_action
TEST args: --packages-above rclcpp_action --packages-above rclcpp_action
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14740

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

@jmachowinski
Copy link
Contributor

@ahcorde Thx for the CI run. The Gist is wrong though, it points to rolling, but must point to humble...

@ahcorde
Copy link
Contributor

ahcorde commented Oct 29, 2024

Pulls: #2635
Gist: https://gist.githubusercontent.com/ahcorde/b6a81a1a37a81db618aed4554f7ff3fe/raw/93c6af7e44abd34eab68bcb9f12196fd45125d2a/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp_action --packages-above-and-dependencies rclcpp_action
TEST args: --packages-above rclcpp_action --packages-above rclcpp_action
ROS Distro: humble
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14741

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

@ahcorde ahcorde requested a review from fujitatomoya October 30, 2024 09:07
@ahcorde ahcorde merged commit 82ec3f0 into ros2:humble Oct 31, 2024
3 checks passed
@urfeex urfeex mentioned this pull request Jan 15, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahcorde ahcorde ahcorde approved these changes

@jmachowinski jmachowinski jmachowinski approved these changes

@fujitatomoya fujitatomoya Awaiting requested review from fujitatomoya

Assignees
No one assigned
Labels
requires-changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@edgarcamilocamacho @mjcarroll @fujitatomoya @jmachowinski @ahcorde