Nodes should exit cleanly

[nnmm]

Currently, sending Ctrl-C to a node makes it exit with code 130 (on a Linux machine). It should instead exit cleanly.

If we follow the rclcpp example, this would be achieved by a signal handler which calls rcl_shutdown().

[jhdcs]

rclcpp appears to achieve this by creating a global signal handler that gets initialized when rclcpp::init is called. We do not have a similar rclrs::init function that creates a global context.

As such, should I make it so that each context has its own signal handler? Or should I work on creating an rclrs::init function as well?

[esteve]

Adding a signal handler for every context seems tricky and a source of problems (what if we have more than one context, will all of them handle the exit signal?). Perhaps a rclrs::init makes sense after all, not a huge fan of global variables, but maybe that's the most pragmatic solution. All solutions I've seen in the past for signal handling (in ROS2 or otherwise) involved some form of a global variable in the end.

[esteve]

The ctrlc crate uses a form of global state, it has an static atomic that just makes sure that set_handler is only called once:

https://github.com/Detegr/rust-ctrlc/blob/master/src/lib.rs#L60=

And then init_os_handler also uses a static variable:

https://github.com/Detegr/rust-ctrlc/blob/master/src/platform/unix/mod.rs#L76=

https://github.com/Detegr/rust-ctrlc/blob/master/src/platform/unix/mod.rs#L14=

In our case, it'd be akin to having a global default context where we'd store the signal handler

[jhdcs]

I had actually been looking at the signal-hook crate: https://crates.io/crates/signal-hook

It would allow us to handle more signals (if needed) than just SIGINT. I'm not sure if that's overkill or not, though. But if I go with that, that would trap us into needing a global default context, right?

[nnmm]

Edit: Removed a part.

There's no fundamental reason we couldn't install a signal handler the first time a context is initialized, by a Once for instance. So I'd try to treat this problem as unrelated from a global default context (which seems very un-Rusty indeed) as far as possible.

[jhdcs]

But then the question in my mind becomes: why didn't rclcpp do that? Why did they gravitate toward a solution that uses a single default context, rather than a signal handler for each potential context?

I'm concerned about the potential possibility of multiple signal handlers somehow potentially stepping on each others' toes. From everything I've read, these things are hard to implement correctly, and the risk of race conditions is high.

Also, when are multiple contexts used? Since rclcpp and rclpy seem to be limited to just one (that I can see), what do we gain from the ability to potentially have multiple?

[nnmm]

I think I heard once that it was because of backwards compatibility with ROS 1, but not sure anymore. I very much agree it's important to understand why other packages do things a certain way.

I'm concerned about the potential possibility of multiple signal handlers somehow potentially stepping on each others' toes. From everything I've read, these things are hard to implement correctly, and the risk of race conditions is high.

I think we wouldn't want to have multiple signal handlers. Since the signal handling crates abstract away things for you, I think there is no more risk of race conditions and it's not hard to implement things correctly. E.g. from taking a quick glance at signal-hook, it seems pretty straightforward to adapt this example to do what we want: Call the signal_hook::flag::register() function inside a Once or so when creating a context, make the term variable a static, and read it inside Context::ok().

Good question about the use case of multiple contexts. But maybe this should be discussed in a new issue?

[jhdcs]

Good question about the use case of multiple contexts. But maybe this should be discussed in a new issue?

Perhaps, but it's tangentially related here - if we want to have a signal handler initialized for each context, wouldn't that make multiple signal handlers need to be created? The way the signal-hook crate shows things being constructed, for example, there would be a flag variable that the processes would have to query. We would either have to create a separate flag variable for each process (which would prevent us from using Once, I think), or we would have to have to have them all share the same flag... Which would be basically a global context, in my mind.

Or am I misunderstanding?

[nnmm]

No, I'm suggesting to only have one global signal handler that gets installed when the first context gets created. We could also uninstall it when the last context gets destroyed. With that in mind, my previous comment hopefully makes more sense.

[jhdcs]

And where would that live? In the other client libraries, they created a global context to store it.

[BrettRD]

rclcpp allows developers to create multiple ROS contexts, this is important when you partition the ROS network with multiple domain IDs, or run loggers from multiple nodes in the same process. The rclcpp global context is still a thing but you can use it indirectly through a local context.
I use multiple rclcpp::Context local-variables in ros-gst-bridge, and I've added the same to my port of fuse, I'd be surprised if it wasn't needed by people implementing interfaces for ros_control

why didn't rclcpp do that?

The rclcpp Node / Excutor / CallbackGroup model had a substantial refactor in Foxy, most of that was shaking off ROS1 baggage of one-process-one-node. I think if you can ditch globals in rclrs, rclcpp will try to adopt your model.

I'll be following this thread with interest because I have a race condition on sigint between rclcpp and GLib

[jhdcs]

All right. With that information, I feel more comfortable about us splitting off from how rclcpp does things.

@nnmm, I will try to see what I can do to get your suggestion working. Let's see if we can get this done without globals!

[nnmm]

@BrettRD Thanks for chiming in. Can you detail what you mean by "you can use it indirectly through a local context" (I thought the global context is an alternative to a local context, where one doesn't use the other) and "ditch globals" (do you mean the global default context?)?

Also, of what nature is the signal handling race condition? Which signal handler is installed first?

@jhdcs My guess is that you'll need at least one static variable. But that's okay imo.

[jhdcs]

I was thinking that, but the static variable would probably be the flag... Which would mean it'd have to be mutable...

And accessing static mut is unsafe... I'm not sure if we can get around that or not, especially because I don't think you're allowed to have a mutex inside a signal handler (so I can't make the flag a static mut term_sig : Arc<Mutex<AtomicBool>>)

While the scope of the problem would be limited to when the program is shutting down, I'd rather the problem not exist at all... Trying to think of a way around it... Perhaps use Once to initialize the signal handler, then modify the Context struct to have a flag field for a terminal signal, and register it with the signal handler each time a Context is created?