rclcpp::spin_some throws exception after SIGINT

[hdino]

Bug report

Required Info:

• Operating System: Ubuntu 18.04
• Installation type: Binaries
• Version or commit hash: 0.8.4-1bionic.20200218.183043
• DDS implementation: Default
• Client library (if applicable): rclcpp

Steps to reproduce issue

There are many examples, for instance the minimal publisher example, which call rclcpp::spin_some in a while loop that checks rclcpp::ok().

However, if the program receives a SIGINT between those two calls, the context might become invalid and spin_some throws an exception:

^C[INFO] [rclcpp]: signal_handler(signal_value=2)
terminate called after throwing an instance of 'rclcpp::exceptions::RCLError'
  what():  Failed to create interrupt guard condition in Executor constructor: the given context is not valid, either rcl_init() was not called or rcl_shutdown() was called., at /tmp/binarydeb/ros-eloquent-rcl-0.8.4/src/rcl/guard_condition.c:69

An easy way to reproduce this behaviour is:

while (rclcpp::ok())
{
    std::this_thread::sleep_for(std::chrono::seconds(1));
    rclcpp::spin_some(ros_node);
}

Expected behavior

I can think of several options:

1. Suggest to put spin_some in a try-catch block.
2. Make spin_some not fail under this condition.
3. Make spin_some atomic, such that it both checks and spins.
   Usage could be: while (rclcpp::spin_some(node)) {}
4. Let the user create an object that prevents the shutdown until it is destructed (e.g. the node object). rclcpp::ok() would still signal that the program should end. This might be related to Executor does not maintain a reference to nodes #726.

[fujitatomoya]

i can reproduce the issue, https://github.com/fujitatomoya/examples/tree/topic-20200417-exception-after-sigin-1066

[fujitatomoya]

since Executor possibly throws exception, Suggest to put spin_some in a try-catch block. ?

[hdino]

Well, one could do something along those lines:

try {
    // ...
    while (rclcpp::ok())
    {
        std::this_thread::sleep_for(std::chrono::seconds(1));
        rclcpp::spin_some(ros_node);
    }
} catch (rclcpp::exceptions::RCLError &e) {
    if (rclcpp::ok()) {
        // handle the exception / report to user
    } else {
        // ignore, ROS is shutting down
    }
}

However, it feels a bit wrong as exceptions should only be thrown in exceptional situations - and an orderly shutdown should not be among them.

@fujitatomoya Thanks for setting up the running example!

[Barry-Xu-2018]

@hdino @fujitatomoya

Since allowing to throw exception inside constructor of Executor and spin_some() doesn't return any value(such as error number), I guess author hope user handle the exception. So this issue isn't a bug.
What do you think ?

[fujitatomoya]

@Barry-Xu-2018

I do not think this is a bug either, i guess that user application needs to take care of that exception. which means that i guess we should close this issue and make issue on https://github.com/ros2/examples.

[hdino]

It's definitely not a bug, but as I said above, it feels wrong when an orderly shutdown throws an exception.

To avoid this behaviour, I'm using the following code now:

rclcpp::executors::SingleThreadedExecutor ros_executor;
while (rclcpp::ok())
{
    std::this_thread::sleep_for(std::chrono::seconds(1));
    ros_executor.spin_node_some(ros_node);
}

I would recommend to use this in the example.

[Barry-Xu-2018]

@fujitatomoya

@Barry-Xu-2018

I do not think this is a bug either, i guess that user application needs to take care of that exception. which means that i guess we should close this issue and make issue on https://github.com/ros2/examples.

Yes. It is better to discuss in https://github.com/ros2/examples/issues.

[Barry-Xu-2018]

@hdino

It's definitely not a bug, but as I said above, it feels wrong when an orderly shutdown throws an exception.

To avoid this behaviour, I'm using the following code now:

rclcpp::executors::SingleThreadedExecutor ros_executor;
while (rclcpp::ok())
{
    std::this_thread::sleep_for(std::chrono::seconds(1));
    ros_executor.spin_node_some(ros_node);
}

I would recommend to use this in the example.

If receive SIGINT before "rclcpp::executors::SingleThreadedExecutor ros_executor;", exception also will be thrown.

Besides, the purpose of example is to guide user how to do. I think author doesn't suggest user writing codes as above way. Otherwise rclcpp::spin_some() is useless.

Anyway, we can create an issue to discuss in https://github.com/ros2/examples/issues and listen to other' opinions. What do you think ?

[hdino]

Yes, I agree that we should discuss it in ros2/examples.

Will close this now in favour of ros2/examples#266.