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

Deprecate ClientGoalHandle::async_result() #1120

Merged
merged 1 commit into from
May 21, 2020

Conversation

jacobperron
Copy link
Member

Fixes #955

There are currently two public APIs for users to get the result of a goal.
This change deprecates one of the APIs, which was considered to be unsafe as
it may result in a race with user-code and raise an exception.

Fixes #955

There are currently two public APIs for users to get the result of a goal.
This change deprecates one of the APIs, which was considered to be unsafe as
it may result in a race with user-code and raise an exception.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron added this to In progress in Foxy via automation May 19, 2020
@jacobperron jacobperron moved this from In progress to Review in progress in Foxy May 19, 2020
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, make sure to test up the stack in case it is being used in an example or something.

Foxy automation moved this from Review in progress to Reviewer approved May 19, 2020
@jacobperron
Copy link
Member Author

jacobperron commented May 19, 2020

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

Edit: The only "new" test failure I see is on Linux

"ros2topic.src.ros2.ros2cli.ros2topic.test.test_echo_pub.test_echo_pub"

But I find it extremely unlikely that a change to rclcpp_action would cause ros2topic to misbehave. I think this change is okay to merge, pending a second reviewer.

@jacobperron
Copy link
Member Author

@ros-pull-request-builder retest this please

@jacobperron jacobperron merged commit 64bdef6 into master May 21, 2020
Foxy automation moved this from Reviewer approved to Done May 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the jacob/deprecate_action_api branch May 21, 2020 18:39
@jacobperron
Copy link
Member Author

Release note added for Foxy: ros2/ros2_documentation#707

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Jul 7, 2020
Fixes ros2#955

There are currently two public APIs for users to get the result of a goal.
This change deprecates one of the APIs, which was considered to be unsafe as
it may result in a race with user-code and raise an exception.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Foxy
  
Done
Development

Successfully merging this pull request may close these issues.

rclcpp_action: async_send_goal makes the goal handle available to caller before making it result aware
3 participants