Clean up action clients in nav2_simple_commander

[m2-farzan]

---

Basic Info

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

---

Description of contribution in a few bullet points

Description of documentation updates required from your changes

• Removes an unwanted warning
• Enables controlled release of resources

---

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

[mergify]

@m2-farzan, 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

[jwallace42]

Looks good! I think you still need to call destroy_node in the python files using the robot navigator.

[SteveMacenski]

I'm not too much of a python ros2 developer, so is destroy_node called automatically by the Node class to need to be called specifically that with snake_case? Or is this a user's need to call this, in that case it should follow our naming conventions here as camcelCase & have its addition documented on the API page of navigation.ros.org / in this package's readme.

Otherwise LGTM

[m2-farzan]

@jwallace42 destroy_node is only required if we want to create a basic_navigator node right after we are done with another one (e.g. in unit tests). This is not the case in our python demos, so I don't think we need to modify them.

[m2-farzan]

@SteveMacenski destroy_node is inherited from Node base class. I couldn't find in docs if it's called automatically or not but this little comment suggests it might be. Either way, it's common practice to use destroy_node to clean up after a node. So I found it best to add a camelCase version while keeping the snake_case version there for compatibility & consistency (i.e. to avoid destroy_node and destroyNode from behaving differently).

In the long term the issue should be addressed in the upstream: ros2/rclpy#524

[SteveMacenski]

I'm fine with that -- I prefer defensive programming

[SteveMacenski]

Thanks for the contribution!