Don't call shutdown() after an exception

[christophfroehlich]

Closes #1383 acc. to the hint given by @firesurfer

[saikishor]

Isn't finally also applied even for the case without exception?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.63%. Comparing base (f64c964) to head (fba7310).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1400      +/-   ##
==========================================
+ Coverage   83.58%   83.63%   +0.04%     
==========================================
  Files         122      122              
  Lines       10990    10984       -6     
  Branches      935      933       -2     
==========================================
  Hits         9186     9186              
+ Misses       1492     1488       -4     
+ Partials      312      310       -2     

Flag	Coverage Δ
unittests	83.63% <100.00%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...est_nodes/publisher_forward_position_controller.py	85.71% <100.00%> (+5.71%)	⬆️
...est_nodes/publisher_joint_trajectory_controller.py	59.09% <100.00%> (+1.56%)	⬆️

... and 2 files with indirect coverage changes

[christophfroehlich]

Isn't finally also applied even for the case without exception?

yes, but spin() never will exit without an exception, am I right? I'm not sure if destroy_node() is necessary at all: The examples on docs.ros.org don't do this, except here:

def main(args=None):
    rclpy.init(args=args)
    avoider = ObstacleAvoider()
    rclpy.spin(avoider)
    # Destroy the node explicitly
    # (optional - otherwise it will be done automatically
    # when the garbage collector destroys the node object)
    avoider.destroy_node()
    rclpy.shutdown()

https://docs.ros.org/en/rolling/Tutorials/Advanced/Simulators/Webots/Setting-Up-Simulation-Webots-Advanced.html

[firesurfer]

Without having the definite answer - my understanding is that you can not exit
rclpy.spin without an exception:

a) Something throws an exception during the spin (e.g. one of your callbacks)
b) Any signal sent to the process also either generates a KeyboardInterrupt or rclpy.executors.ExternalShutdownException

But when would you like to explicitly destroy a node?

Behind the node there is always the Context. rclpy.init implicitly create a global context. In this context you could create and destroy as many nodes as you want as long as the context is valid (which is not the case when we exit with an exception due to a signal - at least that is what I saw so far). But you would have to manage spinning the nodes manually.

The reason why there are methods for destroying publisher/subscription etc. and nodes is that the default garbage collection mechanism of python doesn't work with those. There is always a corresponding native object in the rcl layer and therefore it is necessary to manually release resources. (I've seen that there is some work done in rclpy to implement those as ContextManagers in order to handle it in a better way)

Btw. if you need ROS communication after leaving spin due to an exception it is possible to create a new context and perform any communication inside that.

[christophfroehlich]

Thanks for the background info. Would you say that the current state is fine then? I mean, this is a test node and not productive code, but still like to understand how to properly design this ;)

[firesurfer]

Thanks for the background info. Would you say that the current state is fine then? I mean, this is a test node and not productive code, but still like to understand how to properly design this ;)

See my comment.
At least that's my understanding.

I also think the rclpy documentation is not very clear about this - in addition there are also some things that not work the way they should (see the on_shutdown hook for example)

[saikishor]

Sounds fine. I hope it works the same back in Humble too