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

Make mapUpdateLoop() indicator variables to be thread-safe #3308

Conversation

AlexeyMerzlyakov
Copy link
Collaborator

@AlexeyMerzlyakov AlexeyMerzlyakov commented Dec 2, 2022

This is a elaboration of https://github.com/ros-planning/navigation2/pull/3307/files#r1038041946 comment for #3307.
Costmap2DROS contains one std::thread parallel execution, which calls mapUpdateLoop() routine.

As stated in a C standard docs in a "5.1.2.4 Multi-threaded executions and data races"

The execution of a program contains a data race if it contains two conflicting actions in
different threads, at least one of which is not atomic, and neither happens before the
other. Any such data race results in undefined behavior.

So, all write-write and write-read parallel operations should be protected by mutex, or std::atomic in case if it is the variable.

The mapUpdateLoop() thread contains the following boolean indicator variables appeared to be thread-unsafe:


Basic Info

Info Please fill out this column
Ticket(s) this addresses #3307
Primary OS tested on Ubuntu 20.04
Robotic platform tested on colcon test --packages-select nav2_costmap_2d

Description of contribution in a few bullet points

  • Protected all bool indicator variables in mapUpdateLoop() thread from unsafe usage.

Future work that may be required in bullet points

  • It would be cool to check all places with the std::thread utilization in Nav2, and find all shared variables (bool, int, string, vectors, etc...) and make them to be thread-safe, if they not.

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@mergify
Copy link
Contributor

mergify bot commented Dec 2, 2022

@AlexeyMerzlyakov, please properly fill in PR template in the future. @SteveMacenski, use this instead.

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

God I hate all the stuff going on in this file. I welcome the day I can delete all this and do a rewrite

@SteveMacenski SteveMacenski merged commit a1fd077 into ros-navigation:main Dec 6, 2022
jwallace42 pushed a commit to jwallace42/navigation2 that referenced this pull request Dec 14, 2022
SteveMacenski added a commit that referenced this pull request Dec 21, 2022
* remove exec_depend on behaviortree_cpp_v3 (#3279)

* BT Service Node to throw if service was not available in time (#3256)

* throw if service server wasn't available in time

mimic the behavior of the bt action node constructor

* throw if action unavailable in bt cancel action

* use chrono literals namespace

* fix linting errors

* fix code style divergence

* Remove duplicate of nav2_back_up_cancel_bt_node (#3332)

* Remove unused velocity scaling config from example xml (#3330)

Signed-off-by: Borong Yuan <[email protected]>

Signed-off-by: Borong Yuan <[email protected]>

* Make mapUpdateLoop() indicator variables to be thread-safe (#3308)

* Ensure that plugin initialization to be called before updating routines (#3307)

* Solve bug when CostmapInfoServer is reactivated (#3292)

* Solve bug when CostmapInfoServer is reactivated

* Implemented smoother selector bt node (#3283)

* Implemented smoother selector bt node

Signed-off-by: Owen Hooper <[email protected]>

* updated copyright in modified file

Signed-off-by: Owen Hooper <[email protected]>

Signed-off-by: Owen Hooper <[email protected]>

* Add allow_unknown parameter to theta star planner (#3286)

* Add allow unknown parameter to theta star planner

* Add allow unknown parameter to tests

* missing comma

* Change cost of unknown tiles

* Uncrustify

* bump to 1.1.3 for humble sync 3

Signed-off-by: Borong Yuan <[email protected]>
Signed-off-by: Owen Hooper <[email protected]>
Co-authored-by: Adam Aposhian <[email protected]>
Co-authored-by: Erwin Lejeune <[email protected]>
Co-authored-by: Sven Langner <[email protected]>
Co-authored-by: Borong Yuan <[email protected]>
Co-authored-by: Alexey Merzlyakov <[email protected]>
Co-authored-by: MartiBolet <[email protected]>
Co-authored-by: Owen Hooper <[email protected]>
Co-authored-by: Pedro Alejandro González <[email protected]>
andrewlycas pushed a commit to StratomInc/navigation2 that referenced this pull request Feb 23, 2023
shrijitsingh99 pushed a commit to moss-ag/navigation2 that referenced this pull request Mar 4, 2023
* remove exec_depend on behaviortree_cpp_v3 (ros-navigation#3279)

* BT Service Node to throw if service was not available in time (ros-navigation#3256)

* throw if service server wasn't available in time

mimic the behavior of the bt action node constructor

* throw if action unavailable in bt cancel action

* use chrono literals namespace

* fix linting errors

* fix code style divergence

* Remove duplicate of nav2_back_up_cancel_bt_node (ros-navigation#3332)

* Remove unused velocity scaling config from example xml (ros-navigation#3330)

Signed-off-by: Borong Yuan <[email protected]>

Signed-off-by: Borong Yuan <[email protected]>

* Make mapUpdateLoop() indicator variables to be thread-safe (ros-navigation#3308)

* Ensure that plugin initialization to be called before updating routines (ros-navigation#3307)

* Solve bug when CostmapInfoServer is reactivated (ros-navigation#3292)

* Solve bug when CostmapInfoServer is reactivated

* Implemented smoother selector bt node (ros-navigation#3283)

* Implemented smoother selector bt node

Signed-off-by: Owen Hooper <[email protected]>

* updated copyright in modified file

Signed-off-by: Owen Hooper <[email protected]>

Signed-off-by: Owen Hooper <[email protected]>

* Add allow_unknown parameter to theta star planner (ros-navigation#3286)

* Add allow unknown parameter to theta star planner

* Add allow unknown parameter to tests

* missing comma

* Change cost of unknown tiles

* Uncrustify

* bump to 1.1.3 for humble sync 3

Signed-off-by: Borong Yuan <[email protected]>
Signed-off-by: Owen Hooper <[email protected]>
Co-authored-by: Adam Aposhian <[email protected]>
Co-authored-by: Erwin Lejeune <[email protected]>
Co-authored-by: Sven Langner <[email protected]>
Co-authored-by: Borong Yuan <[email protected]>
Co-authored-by: Alexey Merzlyakov <[email protected]>
Co-authored-by: MartiBolet <[email protected]>
Co-authored-by: Owen Hooper <[email protected]>
Co-authored-by: Pedro Alejandro González <[email protected]>
shrijitsingh99 pushed a commit to moss-ag/navigation2 that referenced this pull request Mar 4, 2023
* remove exec_depend on behaviortree_cpp_v3 (ros-navigation#3279)

* BT Service Node to throw if service was not available in time (ros-navigation#3256)

* throw if service server wasn't available in time

mimic the behavior of the bt action node constructor

* throw if action unavailable in bt cancel action

* use chrono literals namespace

* fix linting errors

* fix code style divergence

* Remove duplicate of nav2_back_up_cancel_bt_node (ros-navigation#3332)

* Remove unused velocity scaling config from example xml (ros-navigation#3330)

Signed-off-by: Borong Yuan <[email protected]>

Signed-off-by: Borong Yuan <[email protected]>

* Make mapUpdateLoop() indicator variables to be thread-safe (ros-navigation#3308)

* Ensure that plugin initialization to be called before updating routines (ros-navigation#3307)

* Solve bug when CostmapInfoServer is reactivated (ros-navigation#3292)

* Solve bug when CostmapInfoServer is reactivated

* Implemented smoother selector bt node (ros-navigation#3283)

* Implemented smoother selector bt node

Signed-off-by: Owen Hooper <[email protected]>

* updated copyright in modified file

Signed-off-by: Owen Hooper <[email protected]>

Signed-off-by: Owen Hooper <[email protected]>

* Add allow_unknown parameter to theta star planner (ros-navigation#3286)

* Add allow unknown parameter to theta star planner

* Add allow unknown parameter to tests

* missing comma

* Change cost of unknown tiles

* Uncrustify

* bump to 1.1.3 for humble sync 3

Signed-off-by: Borong Yuan <[email protected]>
Signed-off-by: Owen Hooper <[email protected]>
Co-authored-by: Adam Aposhian <[email protected]>
Co-authored-by: Erwin Lejeune <[email protected]>
Co-authored-by: Sven Langner <[email protected]>
Co-authored-by: Borong Yuan <[email protected]>
Co-authored-by: Alexey Merzlyakov <[email protected]>
Co-authored-by: MartiBolet <[email protected]>
Co-authored-by: Owen Hooper <[email protected]>
Co-authored-by: Pedro Alejandro González <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants