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

Bugfix 20210810 get current state #1756

Merged
merged 2 commits into from
Oct 28, 2022
Merged

Bugfix 20210810 get current state #1756

merged 2 commits into from
Oct 28, 2022

Conversation

fujitatomoya
Copy link
Collaborator

fix #1746

@fujitatomoya fujitatomoya mentioned this pull request Aug 25, 2021
Closed
clalancette
clalancette requested changes Sep 14, 2021
View reviewed changes
rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp Outdated Show resolved Hide resolved
rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp Outdated Show resolved Hide resolved
rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp Outdated Show resolved Hide resolved
rclcpp_lifecycle/src/state.cpp Outdated Show resolved Hide resolved
@clalancette clalancette added the more-information-needed Further information is required label Sep 30, 2021
@fujitatomoya fujitatomoya force-pushed the bugfix-20210810-get_current_state branch from a9de1b6 to de7ec73 Compare October 22, 2021 22:34
@fujitatomoya
Copy link
Collaborator Author

CI:

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

@fujitatomoya
Copy link
Collaborator Author

@iuhilnehc-ynos @Barry-Xu-2018 could you guys review this if you got time?

iuhilnehc-ynos
iuhilnehc-ynos reviewed Oct 28, 2021
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.

@fujitatomoya
few minor comments. (I might be wrong, if so, please ignore them.)

rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp Outdated Show resolved Hide resolved
rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp Outdated Show resolved Hide resolved
rclcpp_lifecycle/src/state.cpp Outdated Show resolved Hide resolved
@fujitatomoya
Copy link
Collaborator Author

@clalancette @ivanpauno what do you think about this fix? there is still discussion needs to be done for future, but this can fix #1746 (avoid crash and exception.)

@fujitatomoya fujitatomoya force-pushed the bugfix-20210810-get_current_state branch from 99798a1 to 3a3d395 Compare November 16, 2021 17:21
@fujitatomoya fujitatomoya self-assigned this Dec 3, 2021
@fujitatomoya fujitatomoya added bug Something isn't working and removed more-information-needed Further information is required labels Dec 3, 2021
@fujitatomoya fujitatomoya force-pushed the bugfix-20210810-get_current_state branch from 3a3d395 to 131841a Compare December 15, 2021 01:32
clalancette
clalancette requested changes Dec 15, 2021
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left a few more comments, and we still have open the question on what to do with get_current_state. But this is getting closer.

rclcpp_lifecycle/src/state.cpp Show resolved Hide resolved
rclcpp_lifecycle/src/state.cpp Show resolved Hide resolved
rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp Outdated Show resolved Hide resolved
rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp Outdated Show resolved Hide resolved
rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp Outdated Show resolved Hide resolved
@fujitatomoya
Copy link
Collaborator Author

@clalancette thanks, i will look into comments and get back to you.

@fujitatomoya fujitatomoya force-pushed the bugfix-20210810-get_current_state branch from 131841a to d88adf8 Compare December 20, 2021 17:51
@fujitatomoya
Copy link
Collaborator Author

I am still requesting some help for a couple of questions. any comments will do good!

@fujitatomoya
Copy link
Collaborator Author

@clalancette @iuhilnehc-ynos friendly ping.

@iuhilnehc-ynos
Copy link
Collaborator

@fujitatomoya

I don't know if I should continue to review it because returning a copy of State[1,2] seems to be the consensus.
To fix an issue with the correct way rather than a workaround might be better. I know it could affect many source codes or even other repositories.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:21
@fujitatomoya fujitatomoya force-pushed the bugfix-20210810-get_current_state branch from e28ab16 to 2d9e5d5 Compare October 1, 2022 00:00
This was referenced Oct 17, 2022
Merged
Merged
@fujitatomoya @clalancette
protect state_machine_ with mutex lock.
5be565f 
protect state_handle_ with mutex lock.

reconsider mutex lock scope.

remove mutex lock from constructors.

lock just once during initialization of LifecycleNodeInterfaceImpl.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@clalancette clalancette force-pushed the bugfix-20210810-get_current_state branch from 2d9e5d5 to 5be565f Compare October 24, 2022 21:43
clalancette
clalancette approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

With all of the latest fixes, I've now rebased this onto the latest. I think this looks much better now and should fix the original problem. That said, since I did substantial work on this, I'd like @fujitatomoya to take another look at this before we go ahead with it.

@clalancette
Copy link
Contributor

CI:

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

@clalancette
Move updating of current_state to right after initialization.
a46b6ac 
This is slightly more correct in the case that registering one
of the services fails.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@fujitatomoya
Copy link
Collaborator Author

@clalancette thanks i will take a look at this soon.

@fujitatomoya fujitatomoya merged commit e5d13a2 into ros2:rolling Oct 28, 2022
@roncapat
Copy link
Contributor

roncapat commented Mar 21, 2023 •
edited
Loading

@fujitatomoya is this backportable to humble?

Edit:
I had to cherry-pick the following (in order):

In particular, this is useful with gazebo11 + gazebo-ros + gazebo-ros2-control Humble setup, because spurious Error in state! Internal state_handle is NULL. happen rather frequently for me.

@fujitatomoya
Copy link
Collaborator Author

Unfortunately I do not think so, this breaks ABI so cannot land on humble.

@clalancette
Copy link
Contributor

Unfortunately I do not think so, this breaks ABI so cannot land on humble.

It is probably possible to make an ABI-preserving fix by introducing a map of State pointers to mutexes, and then looking up the appropriate mutex in that map as needed. But someone needs to find the time to do that.

alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Mar 24, 2023
@fujitatomoya @clalancette
Bugfix 20210810 get current state (ros2#1756)
86c6cb0 
* protect state_machine_ with mutex lock.

protect state_handle_ with mutex lock.

reconsider mutex lock scope.

remove mutex lock from constructors.

lock just once during initialization of LifecycleNodeInterfaceImpl.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* Move updating of current_state to right after initialization.

This is slightly more correct in the case that registering one
of the services fails.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Mar 24, 2023
@fujitatomoya @clalancette
Bugfix 20210810 get current state (ros2#1756)
1831e12 
* protect state_machine_ with mutex lock.

protect state_handle_ with mutex lock.

reconsider mutex lock scope.

remove mutex lock from constructors.

lock just once during initialization of LifecycleNodeInterfaceImpl.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* Move updating of current_state to right after initialization.

This is slightly more correct in the case that registering one
of the services fails.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
@roncapat roncapat mentioned this pull request Mar 30, 2023
Open
@schornakj schornakj mentioned this pull request Apr 25, 2023
Closed
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023
@fujitatomoya @clalancette
Bugfix 20210810 get current state (ros2#1756)
7885f41 
* protect state_machine_ with mutex lock.

protect state_handle_ with mutex lock.

reconsider mutex lock scope.

remove mutex lock from constructors.

lock just once during initialization of LifecycleNodeInterfaceImpl.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* Move updating of current_state to right after initialization.

This is slightly more correct in the case that registering one
of the services fails.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023
@fujitatomoya @clalancette
Bugfix 20210810 get current state (ros2#1756)
ad9cd1a 
* protect state_machine_ with mutex lock.

protect state_handle_ with mutex lock.

reconsider mutex lock scope.

remove mutex lock from constructors.

lock just once during initialization of LifecycleNodeInterfaceImpl.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* Move updating of current_state to right after initialization.

This is slightly more correct in the case that registering one
of the services fails.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
schornakj added a commit to schornakj/rclcpp that referenced this pull request May 2, 2023
@schornakj
apply changes to LifecycleNodeInterfaceImpl from ros2#1756
b640cad 
Signed-off-by: Joe Schornak <joe.schornak@gmail.com>
@schornakj schornakj mentioned this pull request May 2, 2023
Merged
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request May 3, 2023
@fujitatomoya @clalancette
Bugfix 20210810 get current state (ros2#1756)
6c84b07 
* protect state_machine_ with mutex lock.

protect state_handle_ with mutex lock.

reconsider mutex lock scope.

remove mutex lock from constructors.

lock just once during initialization of LifecycleNodeInterfaceImpl.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* Move updating of current_state to right after initialization.

This is slightly more correct in the case that registering one
of the services fails.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette pushed a commit to schornakj/rclcpp that referenced this pull request Jun 26, 2023
@schornakj @clalancette
apply changes to LifecycleNodeInterfaceImpl from ros2#1756
e8e6d89 
Signed-off-by: Joe Schornak <joe.schornak@gmail.com>
clalancette pushed a commit that referenced this pull request Jun 27, 2023
@schornakj
Fix thread safety in LifecycleNode::get_current_state() for Humble (#…
25263e8 
…2183)

* add initially-failing test case

* apply changes to LifecycleNodeInterfaceImpl from #1756

* add static member to State for managing state_handle_ access

* allow parallel read access in MutexMap

Signed-off-by: Joe Schornak <joe.schornak@gmail.com>
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 left review comments

@Barry-Xu-2018 Barry-Xu-2018 Barry-Xu-2018 left review comments

@ivanpauno ivanpauno ivanpauno left review comments

@clalancette clalancette clalancette approved these changes

Assignees

@fujitatomoya fujitatomoya

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rclcpp_lifecycle::LifecycleNode::get_current_state is not thread safe
6 participants
@fujitatomoya @iuhilnehc-ynos @clalancette @roncapat @ivanpauno @Barry-Xu-2018