fix: Fixed race condition in action server between is_ready and take"… #2531

jmachowinski · 2024-05-14T11:18:08Z

Some background information: is_ready, take_data and execute data may be called from different threads in any order. The code in the old state expected them to be called in series, without interruption. This lead to multiple race conditions, as the state of the pimpl objects was altered by the three functions in a non thread safe way.

This is a clean backport of #2495. This should superseed #2530

Note, this patch is not tested.

@firesufer can you try out this patch ?

firesurfer · 2024-05-14T11:33:45Z

@jmachowinski I just tested it on my local setup and it worked fine so far. I will deploy it on our machine and checkout if it fixes the race condition issue.

rclcpp_action/include/rclcpp_action/server.hpp

jmachowinski · 2024-05-14T14:22:27Z

Hm, we run into a problem with this backport, as #2109 is one of the causes of this bug, and can not be reverted without API break.

As a workaround, I would propose to remove the exception, if take_data is called multiple times, and make execute() handle this case as well. Before going forward with this, I would like to hear you opinions on this @clalancette @wjwwood @mjcarroll

firesurfer · 2024-05-15T12:17:45Z

@jmachowinski Apparently this back port also does not fix the issue completely.

1715775009.7834265 [ros2_control_node-14] terminate called after throwing an instance of 'std::runtime_error'
1715775009.7838161 [ros2_control_node-14] what(): ServerBase::take_data() called but no data is ready
1715775009.7845201 [ros2_control_node-14] Stack trace (most recent call last) in thread 2061428:
1715775009.7851567 [ros2_control_node-14] #14 Object "", at 0xffffffffffffffff, in
1715775009.7882259 [ros2_control_node-14] #13 Object "/usr/lib/x86_64-linux-gnu/libc.so.6", at 0x7f8392da484f, in
1715775009.7885315 [ros2_control_node-14] #12 Object "/usr/lib/x86_64-linux-gnu/libc.so.6", at 0x7f8392d12ac2, in
1715775009.7904732 [ros2_control_node-14] #11 Object "/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30", at 0x7f8392fa5252, in
1715775009.7919304 [ros2_control_node-14] #10 Object "/home/user/workspace/install/rclcpp/lib/librclcpp.so", at 0x7f83932b34d9, in rclcpp::executors::MultiThreadedExecutor::run(unsigned long)
1715775009.7929633 [ros2_control_node-14] #9 Object "/home/user/workspace/install/rclcpp/lib/librclcpp.so", at 0x7f83932a0f8c, in rclcpp::Executor::get_next_executable(rclcpp::AnyExecutable&, std::chrono::duration<long, std::ratio<1l, 1000000000l> >)
1715775009.7959473 [ros2_control_node-14] #8 Object "/home/user/workspace/install/rclcpp/lib/librclcpp.so", at 0x7f8393299066, in rclcpp::Executor::get_next_ready_executable_from_map(rclcpp::AnyExecutable&, std::map<std::weak_ptrrclcpp::CallbackGroup, std::weak_ptrrclcpp::node_interfaces::NodeBaseInterface, std::owner_less<std::weak_ptrrclcpp::CallbackGroup >, std::allocator<std::pair<std::weak_ptrrclcpp::CallbackGroup const, std::weak_ptrrclcpp::node_interfaces::NodeBaseInterface > > > const&)
1715775009.7967491 [ros2_control_node-14] #7 Object "/home/user/workspace/install/rclcpp_action/lib/librclcpp_action.so", at 0x7f8384326569, in rclcpp_action::ServerBase::take_data() [clone .cold]
1715775009.7971637 [ros2_control_node-14] #6 Object "/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30", at 0x7f8392f774d7, in __cxa_throw
1715775009.7974977 [ros2_control_node-14] #5 Object "/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30", at 0x7f8392f77276, in std::terminate()
1715775009.7977948 [ros2_control_node-14] #4 Object "/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30", at 0x7f8392f7720b, in
1715775009.7980995 [ros2_control_node-14] #3 Object "/usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.30", at 0x7f8392f6bb9d, in
1715775009.7983689 [ros2_control_node-14] #2 Object "/usr/lib/x86_64-linux-gnu/libc.so.6", at 0x7f8392ca67f2, in abort
1715775009.7984428 [ros2_control_node-14] #1 Object "/usr/lib/x86_64-linux-gnu/libc.so.6", at 0x7f8392cc0475, in raise
1715775009.7987077 [ros2_control_node-14] #0 Object "/usr/lib/x86_64-linux-gnu/libc.so.6", at 0x7f8392d149fc, in pthread_kill
1715775009.7987759 [ros2_control_node-14] Aborted (Signal sent by tkill() 2061064 1000)

jmachowinski · 2024-05-15T12:33:02Z

@jmachowinski Apparently this back port also does not fix the issue completely.

Yes, this is the problem that I was talking about above.

mjcarroll · 2024-05-21T15:15:57Z

As a workaround, I would propose to remove the exception, if take_data is called multiple times, and make execute() handle this case as well.

Only for the backport or for rolling and jazzy as well?

jmachowinski · 2024-05-21T16:04:10Z

As a workaround, I would propose to remove the exception, if take_data is called multiple times, and make execute() handle this case as well.

Only for the backport or for rolling and jazzy as well?

Only for backport to Iron, for humble this patch should work as it is right now.

firesurfer · 2024-06-18T09:06:01Z

Whats the state with this PR? @mjcarroll @clalancette
This bug is an absolute showstopper for using actions in any kind of industrial application and the fix should definitely be back ported from my point of view.

rclcpp_action/src/client.cpp

jmachowinski · 2024-06-25T09:02:57Z

@firesurfer I updated the patch, to ignore the double calls to take_data, can you test it ?

@clalancette @mjcarroll @wjwwood As this PR has been stale for some time without any response from you the attendees of the Client Working Group Meeting decided, that we will go for just ignoring the double calls to take_data and go on with this PR.

firesurfer · 2024-06-25T09:21:14Z

@jmachowinski Thanks for the patch. I'm rolling it out to our machine right now and will give you feedback this week.

jmachowinski · 2024-06-25T09:31:57Z

@firesurfer one second, I missed a part....

jmachowinski · 2024-06-25T09:46:01Z

Fixed, @firesurfer sorry for the confusion, now the patch should be good to go

firesurfer · 2024-06-25T11:51:35Z

@jmachowinski I guess the issue is fixed on the server side in rclcpp with this fix. On the client side with rclpy we still get a related error. But as I said my guess is that this is not related wit hthe action server side (ros2control - jtc) in this case.

[WARN] [1719316017.260343581] [.action_client]: Ignoring unexpected goal response. There may be more than one action server for the action '<my_jtc>/follow_joint_trajectory'

EDIT: It seems this is also a race condition but in the action implementation in rclpy:
ros2/rclpy#1123

…ros2#2495) Some background information: is_ready, take_data and execute data may be called from different threads in any order. The code in the old state expected them to be called in series, without interruption. This lead to multiple race conditions, as the state of the pimpl objects was altered by the three functions in a non thread safe way. Co-authored-by: William Woodall <william@osrfoundation.org> Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

This adds a workaround for a known bug in the executor in iron. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski · 2024-06-25T13:38:59Z

@alsora @fujitatomoya I guess we are good to go. Can one of you run the CI ?

alsora · 2024-06-25T15:09:54Z

Linux
Linux-aarch64
Linux-rhel
Windows

jmachowinski · 2024-06-25T15:49:51Z

@alsora I think you started the CI for the wrong OS / Distro...

fujitatomoya

lgtm with green CI

Do we not need the test fix https://github.com/ros2/rclcpp/pull/2495/files#diff-b5e37eb4f1e19276cc541af7323bb55cfc86b1e7b2e0fa1f9201f5436d73b65a?

rclcpp_action/src/client.cpp

fujitatomoya · 2024-06-25T21:23:17Z

I believe that this can keep the ABI compatibility but applying ABI compliance checker would be nice.

alsora · 2024-06-26T02:44:23Z

The builds failed due to a strange error in rmw_connextdds_common... All the 380 previous packages built fine..
https://ci.ros2.org/job/ci_linux/21319/consoleFull

The gist was created from https://github.com/ros2/ros2/blob/iron/ros2.repos and it seems correct to me.

jmachowinski · 2024-06-26T11:48:52Z

The gist was created from https://github.com/ros2/ros2/blob/iron/ros2.repos and it seems correct to me.

Here (https://ci.ros2.org/job/ci_linux/21319/) it says:
ubuntu_distro: noble,
el_release: 9,
ros_distro: rolling,

Therefore I assumed the CI is off, as ubuntu should be Jammy and ros_disto: Iron...

alsora · 2024-06-26T15:50:52Z

New CI, this should be using Ubuntu Jammy and ROS 2 iron as a base

Linux
Linux-aarch64
Linux-rhel
Windows

jmachowinski · 2024-06-27T14:19:29Z

@alsora @fujitatomoya merge ?

clalancette · 2024-06-27T14:21:51Z

RHEL shouldn't be failing, but it looks like an infrastructure issue. I'll kick it off again.

fujitatomoya · 2024-06-27T18:15:45Z

@alsora CI is green, i am okay to merge this. can you merge this with your lgtm?

alsora · 2024-06-27T18:46:00Z

Yes, the changes look good to me

ros2#2531) * fix: Fixed race condition in action server between is_ready and take" (ros2#2495) Some background information: is_ready, take_data and execute data may be called from different threads in any order. The code in the old state expected them to be called in series, without interruption. This lead to multiple race conditions, as the state of the pimpl objects was altered by the three functions in a non thread safe way. Co-authored-by: William Woodall <william@osrfoundation.org> Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * fix: added workaround for call to double calls to take_data This adds a workaround for a known bug in the executor in iron. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> --------- Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: William Woodall <william@osrfoundation.org>

* Fixes for intra-process actions (#144) * Fixes for intra-process Actions * Fixes for Clang builds * Fix deadlock * Server to store results until client requests them * Fix feedback/result data race See ros2#2451 * Add missing mutex * Check return value of intra_process_action_send --------- Co-authored-by: Mauro Passerino <mpasserino@irobot.com> * Fix IPC Actions data race (#147) * Check if goal was sent through IPC before send responses * Add intra_process_action_server_is_available API to intra-process Client --------- Co-authored-by: Mauro Passerino <mpasserino@irobot.com> * Fix data race in Actions: Part 2 (#148) * Fix data race in Actions: Part 2 * Fix warning - copy elision --------- Co-authored-by: Mauro Passerino <mpasserino@irobot.com> * fix: Fixed race condition in action server between is_ready and take"… (ros2#2531) * fix: Fixed race condition in action server between is_ready and take" (ros2#2495) Some background information: is_ready, take_data and execute data may be called from different threads in any order. The code in the old state expected them to be called in series, without interruption. This lead to multiple race conditions, as the state of the pimpl objects was altered by the three functions in a non thread safe way. Co-authored-by: William Woodall <william@osrfoundation.org> Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> * fix: added workaround for call to double calls to take_data This adds a workaround for a known bug in the executor in iron. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> --------- Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: William Woodall <william@osrfoundation.org> --------- Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: Mauro Passerino <mpasserino@irobot.com> Co-authored-by: jmachowinski <jmachowinski@users.noreply.github.com> Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com> Co-authored-by: William Woodall <william@osrfoundation.org>

akupferb · 2024-08-20T06:39:36Z

Only for backport to Iron, for humble this patch should work as it is right now.

@jmachowinski Has this fix already been added as a patch in Humble? I continue to encounter this race bug in Humble on Ubuntu 22.

jmachowinski · 2024-08-21T12:08:22Z

@jmachowinski Has this fix already been added as a patch in Humble? I continue to encounter this race bug in Humble on Ubuntu 22.

Nope, feel free to cherry pick the correct version and open a PR for it.

- ros2#2531

* Always publish inter-process on TRANSIENT_LOCAL pubs (#152) * Backport upstream Action fixes - ros2#2531 * Fix actions feedback race - ros2#2451 * Fix data race in Actions: Part 3 --------- Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

… Backport from iron ros2#2531

edgarcamilocamacho · 2024-09-30T22:00:09Z

Hello @akupferb , I just backported the fix from iron branch to humble and created the PR #2635

jmachowinski requested review from ivanpauno, hidmic and wjwwood as code owners May 14, 2024 11:18

clalancette requested changes May 14, 2024

View reviewed changes

rclcpp_action/include/rclcpp_action/server.hpp Outdated Show resolved Hide resolved

jmachowinski force-pushed the action_iron_backport branch from 0221344 to d0610d6 Compare May 14, 2024 14:17

alsora reviewed Jun 19, 2024

View reviewed changes

rclcpp_action/src/client.cpp Outdated Show resolved Hide resolved

alsora reviewed Jun 19, 2024

View reviewed changes

rclcpp_action/src/client.cpp Show resolved Hide resolved

jmachowinski force-pushed the action_iron_backport branch from d0610d6 to 469f5cd Compare June 25, 2024 08:48

jmachowinski force-pushed the action_iron_backport branch from 35523a5 to 5f856b6 Compare June 25, 2024 09:39

firesurfer mentioned this pull request Jun 25, 2024

Race condition in ActionClient.*_async functions which result in a future that never complete ros2/rclpy#1123

Open

Janosch Machowinski and others added 2 commits June 25, 2024 14:27

fix: added workaround for call to double calls to take_data

586c44c

This adds a workaround for a known bug in the executor in iron. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

jmachowinski force-pushed the action_iron_backport branch from 5f856b6 to 586c44c Compare June 25, 2024 12:27

fujitatomoya approved these changes Jun 25, 2024

View reviewed changes

rclcpp_action/src/client.cpp Show resolved Hide resolved

rclcpp_action/src/client.cpp Show resolved Hide resolved

jmachowinski requested a review from clalancette June 26, 2024 11:50

alsora merged commit d588ccb into ros2:iron Jun 27, 2024
2 of 3 checks passed

jmachowinski mentioned this pull request Jul 1, 2024

Executing action server but nothing is ready #2226

Open

mauropasse pushed a commit to mauropasse/rclcpp that referenced this pull request Aug 22, 2024

Backport upstream Action fixes

96e3adf

- ros2#2531

mauropasse mentioned this pull request Aug 22, 2024

Humble backport and new fixes irobot-ros/rclcpp#154

Merged

mauropasse pushed a commit to mauropasse/rclcpp that referenced this pull request Aug 23, 2024

Backport upstream Action fixes

7b44b4c

- ros2#2531

edgarcamilocamacho added a commit to Unlimited-Robotics/third_party_rclcpp that referenced this pull request Sep 29, 2024

fix: Fixed race condition in action server between is_ready and take.…

51a88db

… Backport from iron ros2#2531

edgarcamilocamacho mentioned this pull request Sep 30, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fixed race condition in action server between is_ready and take"… #2531

fix: Fixed race condition in action server between is_ready and take"… #2531

jmachowinski commented May 14, 2024

firesurfer commented May 14, 2024

jmachowinski commented May 14, 2024

firesurfer commented May 15, 2024

jmachowinski commented May 15, 2024

mjcarroll commented May 21, 2024

jmachowinski commented May 21, 2024

firesurfer commented Jun 18, 2024 •

edited

Loading

jmachowinski commented Jun 25, 2024

firesurfer commented Jun 25, 2024

jmachowinski commented Jun 25, 2024

jmachowinski commented Jun 25, 2024

firesurfer commented Jun 25, 2024 •

edited

Loading

jmachowinski commented Jun 25, 2024

alsora commented Jun 25, 2024

jmachowinski commented Jun 25, 2024

fujitatomoya left a comment

fujitatomoya commented Jun 25, 2024

alsora commented Jun 26, 2024

jmachowinski commented Jun 26, 2024

alsora commented Jun 26, 2024 •

edited by clalancette

Loading

jmachowinski commented Jun 27, 2024

clalancette commented Jun 27, 2024

fujitatomoya commented Jun 27, 2024

alsora commented Jun 27, 2024

akupferb commented Aug 20, 2024

jmachowinski commented Aug 21, 2024 •

edited

Loading

edgarcamilocamacho commented Sep 30, 2024

fix: Fixed race condition in action server between is_ready and take"… #2531

fix: Fixed race condition in action server between is_ready and take"… #2531

Conversation

jmachowinski commented May 14, 2024

firesurfer commented May 14, 2024

jmachowinski commented May 14, 2024

firesurfer commented May 15, 2024

jmachowinski commented May 15, 2024

mjcarroll commented May 21, 2024

jmachowinski commented May 21, 2024

firesurfer commented Jun 18, 2024 • edited Loading

jmachowinski commented Jun 25, 2024

firesurfer commented Jun 25, 2024

jmachowinski commented Jun 25, 2024

jmachowinski commented Jun 25, 2024

firesurfer commented Jun 25, 2024 • edited Loading

jmachowinski commented Jun 25, 2024

alsora commented Jun 25, 2024

jmachowinski commented Jun 25, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Jun 25, 2024

alsora commented Jun 26, 2024

jmachowinski commented Jun 26, 2024

alsora commented Jun 26, 2024 • edited by clalancette Loading

jmachowinski commented Jun 27, 2024

clalancette commented Jun 27, 2024

fujitatomoya commented Jun 27, 2024

alsora commented Jun 27, 2024

akupferb commented Aug 20, 2024

jmachowinski commented Aug 21, 2024 • edited Loading

edgarcamilocamacho commented Sep 30, 2024

firesurfer commented Jun 18, 2024 •

edited

Loading

firesurfer commented Jun 25, 2024 •

edited

Loading

alsora commented Jun 26, 2024 •

edited by clalancette

Loading

jmachowinski commented Aug 21, 2024 •

edited

Loading