fix bug mentioned in #3958

[GoesM]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#3971
Primary OS tested on	Ubuntu
Robotic platform tested on	turtlebot

---

Description of contribution in a few bullet points

Description of documentation updates required from your changes

bug description

costmap is set as a Nav2::util LifecycleNode : ../nav2_costmap_2d/include/nav2_costmap_2d/costmap_2d_ros.hpp#L 73

all LifecycleNode would react to Ctrl+C ../nav2_util/src/lifecycle_node.cpp#L 90

all LifecycleNode would react to rcl_preshutdown randomly, and similar issue has been discussed before : ../ros2/rclcpp/issues/2096

this explanation should have been clear enough: ../nav2_planner/src/planner_server.cpp #L 214-L225

the bug happening in ISSUE: #3958 is caused by this.

because during shutdown_period, costmap_ros_ may react rcl_preshutdown earlier, so that planner/controller may try to access the costmap_ros_ though it has been a nullptr.

our provided solution

it seems that costmap is only used as a child-thread of planner/controller, ../nav2_planner/src/planner_server.cpp#L 89

so can we make it do nothing when reacting to Ctrl+C ?

doing so, its lifecycle would be completely bond to planner/controller's lifecycle, and also going to death in order.

//in file `costmap_2d_ros.hpp`
  /**
   * @brief override on_rcl_preshutdown() as empty
   * [reason] costmap only could be created by its parents like planner/controller_server
   * [reason] costmap may react to ctrl+C earlier than its parents to cause nullptr-accessed
   * so the costmap's reaction must be later than its parent to avoid nullptr-accessed
   * 
   * thus, it's a more perfect way to override on_rcl_preshutdown() as empty function
   * and its deactivate must be joint in planner/controller_server->on_deactivate()
   * and its cleanup must be joint in planner/controller_server->on_cleanup()
   * 
   * !!! a mention !!!
   * if any other place use this class (Costmap2DROS)
   * it's necessary to let costmap->deactivate() joint in parent->deactivate()
   * and let costmap->cleanup() joint in parent->cleanup() 
   */
  void on_rcl_preshutdown() override{
    //do nothing
    return ;
  }

because the costmp_ros_ is just a thread created by planner/controller,

whether program shutdown correctly or not, or even dead unexpectably, the child-thread could be closed all the time since parent-LifecycleNode sub-process dead.

Thus there's no need to worry whether it would be still alive after whole program exit finally.

in addition

if our solution being adopted, many checks could be removed :

• double check in planner_server->on_deactivate()

• double check in planner_server->on_cleanup()

• double check in controller_server->on_deactivate()

• double check in controller_server->on_cleanup()

these double checks are also designed to face to bugs in situation that costmap_ros_ may be closed earlier than planner/controller. After our code modification, these checks are useless anymore.

what's more, our solution performs perfectly during our local tests, much more than our past solutions mentioned in #3958

---

Future work that may be required in bullet points

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

[SteveMacenski]

I think this is a clever and fine solution to the problem, just some questions to answer

[GoesM]

We specially name nodes like costmap_ros_ as child-LifecycleNode, which is born from their parent-LifecycleNode like planner_server and controller_server.

our suggestion is:
Could we set up a mechanism specially for such situations like child-LifecycleNode, to make sure they're completely controlled by their parent?

these child-LifecycleNode should have some basic specialist like:

• they must be created by a parent-LifecycleNode
• they must be created as a thread but not a sub-process

such mechanism could be like:

• child's->activate should be joint into its their parent's on_configure
• child->deactivate() must be joint into its parent's on_deactivate
• child->cleanup() must be joint into its parent's on_cleanup
• and so on

Perhaps such mechanism could be used for any program in ROS2 ?

[GoesM]

At present, the unit test of CI takes nav2_ costmap_2d is considered as an independent LifecycleNode for launch and shutdown tests, which goes against the premise of our proposed modification proposal

these child-LifecycleNode should have some basic specialist like:
-- they must be created by a parent-LifecycleNode
-- they must be created as a thread but not a sub-process

For this, we have three suggestions to address different development needs that may exist in the future.

1. modify the unit-test

If it can be guaranteed that in current nav2 program usage and subsequent development,
nav2_costmap will only be used as a child-LifecycleNode,
the next step should be to modify the unit test.

It's only in need to modify the launch method of costmap based on the original unit test.
Declare and launch a virtual parent-LifecycleNode, through which to run and preshutdown costmap in tests (just like how planner/controller do it)

2: create a new class ChildCostmap2DROS for NAV2

if you're worried that Costmap2DROS may be used as an individual LifecycleNode in future work,
our suggestion is to create a new class named as ChildCostmap2DROS, specifically to implement the special usage of child-LifecycleNode by planner/controller.

It's only in need to create some new files like child_costmap_2d_ros.hpp similar to costmap_2d_ros.hpp, and child_*.cpp simliar to *.cpp

and change declaration of global_costmap_ and local_costmap_ in planner/controller_server.cpp

3. create a new class ChildLifecycleNode for ROS2

In other ROS2 programs, there may be something else, just like costmap, utilizes the characteristics of LifecycleNode for performance optimization, but is only used as a sub-thread?
if such utilize method is widely used by various ROS2 programs,
our suggestion is to create a new class named as ``ChildLifecycleNode` for whole ROS2, to optimize the shutdown mechanism of all child-LifecycleNodes and parent-LifecycleNodes, for thread management optimization.

by the way, such class ChildLifecycleNode must comply with the following mandatory requirements when used:

1. The 'activate', 'deactivate', and 'cleanup' of this class must be called by a parent-LifecycleNode
2. This class must be executed at the thread level and not be executed at the process level

[GoesM]

If there is anymore work that requires our assistance, we'd like to have a try within our capabilities O(∩_∩)O

[SteveMacenski]

If there is anymore work that requires our assistance, we'd like to have a try within our capabilities O(∩_∩)O

What might you be interested in doing? I can always certainly find projects :-) Taking a stab at #3303 would be very helpful - Alexey made some changes to rclcpp::Rate to make it better supportable of different clocks https://github.com/AlexeyMerzlyakov/rclcpp/blob/rolling/rclcpp/include/rclcpp/rate.hpp#L141

[GoesM]

I can always certainly find projects :-) Taking a stab at #3303 would be very helpful - Alexey made some changes to rclcpp::Rate to make it better supportable of different clocks https://github.com/AlexeyMerzlyakov/rclcpp/blob/rolling/rclcpp/include/rclcpp/rate.hpp#L141

WOW! That's cool ! Thanks a lot ~

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6d2278e) 90.20% compared to head (4d312ab) 90.27%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3972      +/-   ##
==========================================
+ Coverage   90.20%   90.27%   +0.06%     
==========================================
  Files         417      417              
  Lines       18605    18608       +3     
==========================================
+ Hits        16783    16798      +15     
+ Misses       1822     1810      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[GoesM]

if only considering current nav2, we're very sure that such uncovered checks could be removed.

However, perhaps it may be still worried that any future changes may make these checks work again.

anyway, whether remove or retain them, I am very willing to make modifications based on your futher suggestions (●'◡'●)

[SteveMacenski]

LGTM in concept, but we can play some games and make it cleaner

[GoesM]

The failure of CI test may show that some unit tests may need to change their constructor choice ?

[SteveMacenski]

Suggested change

	* Thus, it's neccessary to set NodeOption is_lifecycle_follower_ as true
	* Thus, it's neccessary to set NodeOption is_lifecycle_follower_ as true

Delete.

[SteveMacenski]

Sorry, just some changes to be clear in technical language, Delete that last line and that should be good

[GoesM]

great of thanks for your patience ~

[SteveMacenski]

@GoesM I retriggered CI a few times, lifecycle_test is failing deterministically with this PR. I think you need to look into that

[SteveMacenski]

Great! Thanks for the iteration!