add start_canceled bool flag when creating a timer

[alsora]

This can be used to make sure that a timer is canceled before being registered with a node, avoiding the risk of race conditions if an executor tried to immediately trigger it

Consider a situation where a user creates a timer but wants to activate it only at a later moment.

auto my_executor = std::make_shared<SingleThreadedExecutor>();
std::thread spin_thread([my_executor]() {
    my_executor->spin();
});
my_executor->add_node(my_node);
auto timer = rclcpp::create_timer(my_node, ....);
timer->cancel();

As soon as the timer object is created, the rclcpp::create_timer function would add it to the node timers interface, which would result in notifying the executor about this new entity.
In case of thread starvation and/or race conditions, the executor may try to trigger the timer before it has been cancelled.

This could result in problems or crashes if the users were expecting that timer to not be called at that time.

This PR adds a boolean flag to the rclcpp::create_timer functions in order to cancel the timer in a safe way before adding it to the node timers interface.

[fujitatomoya]

additional test case would be nice to have.

[fujitatomoya]

how about the following? just a suggestion.

Suggested change

	bool start_canceled = false)
	bool auto_start = true)

[fujitatomoya]

It would be probably better to add the test case that

1. create canceled timer.
2. reset timer. (activate)
3. check timer callback is fired.

[tgroechel]

Is this a duplicate of #2005?

[alsora]

Thanks for catching it!
Should we close this one and resurrect the other one? Or should I just apply the requested changes here?

[tgroechel]

Should we close this one and resurrect the other one?

From a quick glance, it looks like the linked issue is part of a somewhat connected "web" of timer related issues. Edit* quick search + links below

Timer issues -> PRs:

• Create a timer that does not get triggered as soon as it is initialized. #1980
  • Modifies timers API to select autostart state #2005: rclcpp add timer autostart option (default is true)
  • Modifies timers API to select autostart state rcl#1004: rcl adds timer autostart option (default is true)
  • Changes ros_timer_init for ros_timer_init2 rclpy#999: rclpy updates timer to use true for autostart
  • Changes ros_timer_init for ros_timer_init2 system_tests#508 systems_tests: updates timer to use true and rename init func
• A timer that can be triggered a finite number of times. #1990
  • Timer that can be triggered a finite number of times. #2006
    • includes long discussion around timer features more generally
    • Timer that can be triggered a finite number of times. #2006 (comment): flow diagram of timers

My only initial concern with keeping the other issue is it seems* a bit stale / older and this one could likely be merged more quickly. Edit* However, they aren't really that old (Dec of last year) and also include rcl, rclpy, & systems_tests changes + include a lot of design discussions.

Or should I just apply the requested changes here?

With either choice (keeping this or the other), I agree with #2194 (comment) that bool auto_start = true is slightly easier to understand/intuitive as it is more semantically inline with/idiomatic of default behavior imo.

[fujitatomoya]

@alsora how about closing this one and working on these open PRs? sorry i should have told you before...