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

Remake of "fix: Fixed race condition in action server between is_ready and take" #2495

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Remake of "fix: Fixed race condition in action server between is_ready and take" #2495

merged 2 commits into from
Apr 9, 2024

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Apr 9, 2024

See: #2250

Janosch Machowinski and others added 2 commits April 4, 2024 22:47
fix: Fixed race condition in action server between is_ready and take_…
34ec478 
…data and execute

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.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: Janosch Machowinski <j.machowinski@nospam.org>
@wjwwood
style fixups
d864f81 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood self-assigned this Apr 9, 2024
@wjwwood wjwwood mentioned this pull request Apr 9, 2024
Closed
@wjwwood wjwwood changed the title Remake of https://github.com/ros2/rclcpp/pull/2250 Remake of "fix: Fixed race condition in action server between is_ready and take" Apr 9, 2024
@wjwwood
Copy link
Member Author

wjwwood commented Apr 9, 2024

New CI:

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

@wjwwood
Copy link
Member Author

wjwwood commented Apr 9, 2024

I can't approve my own pr, but lgtm. I think this will be good to merge if the CI comes back clean.

@mjcarroll
Copy link
Member

Looks like all of the generated typesupport linting failures here, I think we are good to go.

@mjcarroll mjcarroll merged commit dd81ef2 into rolling Apr 9, 2024
2 of 3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the jmachowinski_2250 branch April 9, 2024 23:45
@jmachowinski jmachowinski mentioned this pull request Apr 10, 2024
Closed
@saikishor saikishor mentioned this pull request Apr 29, 2024
Open
@firesurfer firesurfer mentioned this pull request May 14, 2024
Closed
jmachowinski pushed a commit to cellumation/rclcpp that referenced this pull request May 14, 2024
@wjwwood
fix: Fixed race condition in action server between is_ready and take" (…
5bc9abd 
…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.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: Janosch Machowinski <j.machowinski@nospam.org>
Co-authored-by: Janosch Machowinski <j.machowinski@nospam.org>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
jmachowinski pushed a commit to cellumation/rclcpp that referenced this pull request May 14, 2024
@wjwwood
fix: Fixed race condition in action server between is_ready and take" (…
0221344 
…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>
@jmachowinski jmachowinski mentioned this pull request May 14, 2024
Merged
jmachowinski pushed a commit to cellumation/rclcpp that referenced this pull request May 14, 2024
@wjwwood
fix: Fixed race condition in action server between is_ready and take" (…
d0610d6 
…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>
jmachowinski pushed a commit to cellumation/rclcpp that referenced this pull request Jun 25, 2024
@wjwwood
fix: Fixed race condition in action server between is_ready and take" (…
469f5cd 
…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>
jmachowinski pushed a commit to cellumation/rclcpp that referenced this pull request Jun 25, 2024
@wjwwood
fix: Fixed race condition in action server between is_ready and take" (…
cfd020c 
…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>
alsora pushed a commit that referenced this pull request Jun 27, 2024
@jmachowinski @wjwwood
fix: Fixed race condition in action server between is_ready and take"… (
d588ccb 
#2531)

* fix: Fixed race condition in action server between is_ready and take" (#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>
mauropasse pushed a commit to mauropasse/rclcpp that referenced this pull request Aug 9, 2024
@jmachowinski @wjwwood
fix: Fixed race condition in action server between is_ready and take"… (
f165a25 
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>
mauropasse pushed a commit to mauropasse/rclcpp that referenced this pull request Aug 13, 2024
@jmachowinski @wjwwood
fix: Fixed race condition in action server between is_ready and take"… (
f6de20e 
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>
apojomovsky pushed a commit to irobot-ros/rclcpp that referenced this pull request Aug 13, 2024
@mauropasse @jmachowinski @wjwwood
Iron: Action IPC Fixes (#151)
7a51f00 
* 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>
edgarcamilocamacho added a commit to Unlimited-Robotics/third_party_rclcpp that referenced this pull request Sep 29, 2024
@edgarcamilocamacho
fix: Fixed race condition in action server between is_ready and take.…
79e2a87 
… Backport from rolling ros2#2495
@edgarcamilocamacho edgarcamilocamacho mentioned this pull request Sep 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

@ahcorde ahcorde ahcorde approved these changes

@alsora alsora alsora approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno ivanpauno is a code owner

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner

Assignees

@wjwwood wjwwood

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wjwwood @mjcarroll @ahcorde @alsora