Something odd is this makes rclpy nodes not exit on receiving SIGTERM. Before, sending SIGTERM will cause the process to exit like this:

Thanks for noticing that, I didn't think about that issue.

Maybe SIGTERM could additionally trigger a shutdown call on all of the Context instances?

Yes, that's an option.
The other one I'm thinking of is to raise an exception from the signal handler, that's possible using the python signal module: https://docs.python.org/3/library/signal.html#example.

We could maybe have ShutdownError exception in rclpy, if that doesn't exist already.

Now when running the demo_nodes_py listener demo and sending sigterm the following exception is thrown:

  File "/home/ivanpauno/ros2_ws/install/demo_nodes_py/lib/demo_nodes_py/listener", line 11, in <module>
    load_entry_point('demo-nodes-py', 'console_scripts', 'listener')()
  File "/home/ivanpauno/ros2_ws/build/demo_nodes_py/demo_nodes_py/topics/listener.py", line 36, in main
    rclpy.spin(node)
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/__init__.py", line 213, in spin
    executor.spin_once()
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/executors.py", line 704, in spin_once
    handler, entity, node = self.wait_for_ready_callbacks(timeout_sec=timeout_sec)
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/executors.py", line 690, in wait_for_ready_callbacks
    return next(self._cb_iter)
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/executors.py", line 592, in _wait_for_ready_callbacks
    raise ExternalShutdownException()

which seems like a reasonable error.

I tried to use the python signal module for all signal handlers, but it didn't work as I expected.
The issue is that waitset is still waiting for an event, with the GIL locked, and the python signal handler is not going to be run until that returns.

Maybe, we should be using an approach similar to rclcpp: having an extra thread waiting on a semaphore, trigger it in a signal safe fashion, and from that thread trigger guard conditions and shutdown contexts.

The issue is that waitset is still waiting for an event, with the GIL locked, and the python signal handler is not going to be run until that returns.

That's interesting - the intended behavior is to release the GIL while the waitset is waiting. Is there another spot blocking?

Another downside is the Pure python signal handler can only be unregistered from the same thread that registered it (#728 (comment)). Maybe that's less of a problem with this PR registering and unregistering it in rclpy.init/shutdown.

That's interesting - the intended behavior is to release the GIL while the waitset is waiting. Is there another spot blocking?

Sorry, the GIL is not blocked.

But the waitset is blocked waiting on a condition variable.
The python signal module is not "completely asynchronous", a flag will be set from the C signal handler and the python signal handler will be executed the next time the interpreter can check that flag, which is not possible while a cpython extension function is being executed.

In this case, it seems we really need a C signal handler to at least trigger a condition variable.
The issue is that triggering a condition variable is not signal safe, what we are doing in rclcpp is to do that from a thread only used for signal handling, and use a signal safe synchronization mechanism to wake that thread.
That's a preexistent issue though.

I've used a python sigterm handler only for shutting down contexts, which is safer than doing that from C, though it adds more complexity.
In rclcpp, we're doing that from the same thread that trigger the guard conditions, so maybe we could do that here as well.

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

demo_nodes_py talker segfaults when receiving SIGTERM. Is that a sign of an issue here?

Yes, I will test that.

demo_nodes_py talker segfaults when receiving SIGTERM. Is that a sign of an issue here?

It seems that when I tested on Friday I was sending the signal to the ros2cli process launching the node instead that to the node itself.
c032473 should've fixed the issue.
A failure after sigterm is send now looks like:

Traceback (most recent call last):
  File "install/demo_nodes_py/lib/demo_nodes_py/listener", line 11, in <module>
    load_entry_point('demo-nodes-py', 'console_scripts', 'listener')()
  File "/home/ivanpauno/ros2_ws/build/demo_nodes_py/demo_nodes_py/topics/listener.py", line 36, in main
    rclpy.spin(node)
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/__init__.py", line 224, in spin
    executor.spin_once()
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/executors.py", line 704, in spin_once
    handler, entity, node = self.wait_for_ready_callbacks(timeout_sec=timeout_sec)
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/executors.py", line 690, in wait_for_ready_callbacks
    return next(self._cb_iter)
  File "/home/ivanpauno/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/executors.py", line 592, in _wait_for_ready_callbacks
    raise ExternalShutdownException()
rclpy.executors.ExternalShutdownException

Force-pushed to fix DCO

• Linux
• Linux-aarch64
• macOS
• Windows

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

As you hit on in the conversation above, I have one concern with the behavior of this PR, and it's that you can't handle SIGINT and SIGTERM the same within your Python application. If I make a test as follows:

import rclpy
from demo_nodes_py.topics.talker import Talker

def main():
    rclpy.init()
    node = Talker()
    try:
        rclpy.spin(node)
    except KeyboardInterrupt:
        print('Got SIGINT')
    except rclpy.executors.ExternalShutdownException:
        print('Got SIGTERM')
    print('Destroying')
    node.destroy_node()
    rclpy.shutdown()


if __name__ == '__main__':
    main()

If I run this program, and send a SIGINT to the process, then the following happens:

[INFO] [1635816254.444592338] [talker]: Publishing: "Hello World: 0"
Got SIGINT
Destroying

However if you send a SIGTERM, then the following unclean shutdown happens:

[INFO] [1635815908.502049088] [talker]: Publishing: "Hello World: 0"
Got SIGTERM
Destroying
Traceback (most recent call last):
  File "./src/emersonknapp/ektest/src/ekpy", line 42, in <module>
    main()
  File "./src/emersonknapp/ektest/src/ekpy", line 38, in main
    rclpy.shutdown()
  File "/ws/install/gcc/rclpy/lib/python3.8/site-packages/rclpy/__init__.py", line 123, in shutdown
    _shutdown(context=context)
  File "/ws/install/gcc/rclpy/lib/python3.8/site-packages/rclpy/utilities.py", line 58, in shutdown
    return context.shutdown()
  File "/ws/install/gcc/rclpy/lib/python3.8/site-packages/rclpy/context.py", line 101, in shutdown
    self.__context.shutdown()
rclpy._rclpy_pybind11.RCLError: failed to shutdown: rcl_shutdown already called on the given context, at /ws/src/ros2/rcl/rcl/src/rcl/init.c:241

I don't think it makes sense to require different shutdown logic between these signals - did you come to a resolution for which one of the behaviors it should be?

I don't think it makes sense to require different shutdown logic between these signals - did you come to a resolution for which one of the behaviors it should be?

I'm honestly not sure.

Python raises a KeyboardInterrupt from the sigint handler, the signal handler added by ros2 tried to respect that logic.
That seems reasonable, as it's familiar to python users.

We could also raise a KeyboardInterrupt from sigterm. But IMO that would confuse python users, who expect KeyboardInterrupt to only be raised when sigint happens.

Maybe we could raise a different exception when sigterm happens, which would be pretty similar to the sigint case (e.g. rclpy.executors.ExternalShutdownException, without actually shutting down contexts).
It's a bit problematic though, as for that we need the signal module, and signal.signal() can only be called from the main thread, which adds a limitation to rclpy.init() that currently doesn't exist.

The following code should work cleanly in both cases:

import rclpy
from demo_nodes_py.topics.talker import Talker

def main():
    rclpy.init()
    node = Talker()
    try:
        rclpy.spin(node)
    except KeyboardInterrupt:
        print('Got SIGINT')
        rclpy.shutdown()
    except rclpy.executors.ExternalShutdownException:
        print('Got SIGTERM')
    print('Destroying')
    node.destroy_node()


if __name__ == '__main__':
    main()

I think that the other alternative is to also shutdown asynchronously when sigint happens, so you don't really need to call rclpy.shutdown() in either case.
That would be a breaking change, though I think it's probably a good idea.

IIRC @hidmic preferred this alternative.

I think it makes sense for the signals that were configured as a shutdown event upon rclpy initialization to effectively shutdown. That's not mutually exclusive (or shouldn't be IMO) with the default signal handler behavior e.g. raise KeyboardInterrupt on SIGINT.

I think that the other alternative is to also shutdown asynchronously when sigint happens, so you don't really need to call rclpy.shutdown() in either case.
I think it makes sense for the signals that were configured as a shutdown event upon rclpy initialization to effectively shutdown.

Yeah, this also makes sense to me. Then, in practice would we say a ROS 2 Python __main__ should look like:

try:
    rclpy.init()
    my_program()
except (KeyboardInterrupt, rclpy.executors.ExternalShutdownException):
    print('Got clean shutdown signal exception.')
else:
    rclpy.shutdown()
finally:
    my_specific_cleanup()

Yeah, this also makes sense to me. Then, in practice would we say a ROS 2 Python main should look like:

Exactly.
I will open a PR trying that, I guess I will have to open some PRs in other repos fixing regressions.