rclcpp: Asynchronously shutting down while using `rclcpp::spin` `rclcpp:spin_some` in another thread throws an exception

We could instead say that shutdown changes nothing but a flag in the context, and everything continues as normal and it’s up to the user to notice and react to it.

Quoting myself, I think this might be the right path to take. Just make it so that shutdown does not affect the function of anything, it simply changes the state of rclcpp::Context::ok()/rclcpp::ok().

This runs the risk of the user not noticing shutdown, but we can’t keep them from all pitfalls.

@ivanpauno

I also think that we should add the try/catch in our examples/demos.

Just FYI, I think that I already did it with ros2/examples#270.

That’s exactly what I don’t like of how shutdown works right now, you only can try/catch a generic RCLError. There should be a rclcpp::exceptions::Shutdown error (or similar).

i guess we got to the consensus.

Yeap, I will check in tomorrow’s meeting if everybody agrees.

Doing this change will involve coordinated work in rcl, rclpy and rclcpp to make sure that shutdown doesn’t do more than triggering guard conditions once, invalidating the context, and calling shutdown callbacks. The only place where we should check if a context is valid or not is in rclcpp::ok(), rclpy.ok() (e.g. publish should continue working, etc).

Getting the refactor right might be a bit tricky, if you’re planning to do I can help with the reviews.

@ivanpauno

thanks for the clarification, we will take care of this.

Sorry, but up to now I’m the only one in favor of that change, we need to settle in what we want first. I would like to hear other opinions before starting a change that may be rejected.

@wjwwood @hidmic @jacobperron Does a custom return code in rcl and a custom exception in rclcpp for shutdown sound like a good idea to solve the issue? Do you have any other alternative?

i think that is feasible. one question, what is the difference between RCL_RET_IS_SHUTDOWN and RCL_RET_CONTEXT_INVALID?

Sorry, I think my comment wasn’t clear.

Those are two names suggestions for the same new return code. RCL_RET_CONTEXT_INVALID is named similarly to the other return codes. RCL_RET_IS_SHUTDOWN is a more clear name for this case IMO.

We already have in rcl return values like RCL_RET_NODE_INVALID, RCL_RET_PUBLISHER_INVALID, etc.

After shutdown, you usually get one of those errors (e.g. https://github.com/ros2/rcl/blob/069d1f07290019b62a8297c5f95a80b58e63682a/rcl/src/rcl/publisher.c#L286). Can we add a RCL_RET_IS_SHUTDOWN or RCL_RET_CONTEXT_INVALID in rcl and return that when appropriate?

We could then add in rclcpp a ShutdownError derived from RCLError and make sure we throw that one in throw_from_rcl_error. In rclpy, we should make sure to always raise KeybordInterrupt in those cases.

I think that with those changes, we will be able to handle an asynchronous shutdown gracefully.

This approach requires careful reviewing of all functions in rcl layer, to make sure they are returning the appropriate error (and careful handling of those errors in the upper client layer).

Note: It’s impossible to recognize an already shutdown context of one that was never init, that should be clear in error messages.