Rclcpp: Timer class methods aren't thread safe

[ivanpauno]

Feature request

Feature description

Methods are using timer_handle_ member variable without adding any kind of thread-safe protection, e.g.: https://github.com/ros2/rclcpp/blob/96ebf59a6045a535730d98fff25e522807c7aa75/rclcpp/src/rclcpp/timer.cpp#L83

rcl functions aren't thread safe, so a std::mutex (or maybe std::recursive_mutex) should be added to protect concurrent access of timer_handle_.

See https://github.com/ros2/rclcpp/pull/1013#issuecomment-597621877.

Implementation considerations

[rotu]

Could you please add the description of what the bug is to this issue description?

[rotu]

I mean in the top post - the top comment right now is basically just a link, and well, it should describe the issue

What functions aren't threadsafe that should be? All instance methods of rclcpp::TimerBase?

Are there any observed behavioral problems or test failures that have been linked with this issue?

[ivanpauno]

I mean in the top post - the top comment right now is basically just a link, and well, it should describe the issue

Done. If you read the old comment the post description was linking, it says almost the same thing.

What functions aren't threadsafe that should be? All instance methods of rclcpp::TimerBase?

All the ones that access this member variable:
https://github.com/ros2/rclcpp/blob/96ebf59a6045a535730d98fff25e522807c7aa75/rclcpp/include/rclcpp/timer.hpp#L106

Are there any observed behavioral problems or test failures that have been linked with this issue?

No, my complain is just theoretical now.
Several methods are accessing and modifying a data structure (making using of rcl functions), without protecting from concurrent access.
It might not have been detected as an issue now. It can segfault, hang or behave strangely in the future.

[wjwwood]

I doubt I get to this before the API freeze, What's the expectation here for triage? I don't think we need to investigate anything right? I'd either put it on the foxy board or backlog. If on the foxy board we can randomly assign it to me, but again, it will be after API freeze before I have any time.

[rotu]

FYI: not directly related to timers, but I did catch a case where the rclcpp API (rclcpp::Node constructor)'s lack of thread-safety caused a hard crash. I got lucky in that it was readily reproducible and visible.

I think we need a broader effort to instrument and report unsafe usage of functions that are not threadsafe.

https://github.com/ros2/rosbag2/issues/329

[audrow]

@ivanpauno

I believe that the timer class is thread safe because all of the functions that use timer_handle_ say that they are thread safe in their documentation, except for rcl_timer_init which is in the constructor and is locked with a mutex from the clock. Below are the functions and the documentation that states they are thread safe.

I'm not sure why some of the code-blocks below aren't importing correctly.

timer_handle_ usages in timer.cpp

rcl_timer_init

This function is in the constructor and has a lock from the clock.

https://github.com/ros2/rclcpp/blob/cc65905efa5e2b7f2ba3d3c5ce0a69a716b95630/rclcpp/src/rclcpp/timer.cpp#L62-L74

rcl_timer_cancel

https://github.com/ros2/rclcpp/blob/cc65905efa5e2b7f2ba3d3c5ce0a69a716b95630/rclcpp/src/rclcpp/timer.cpp#L83

Documentation:

https://github.com/ros2/rcl/blob/ffcfda18db7aa9ad7c01ebdd26ae7bdd73c8206a/rcl/include/rcl/timer.h#L465-L491

rcl_timer_is_canceled

https://github.com/ros2/rclcpp/blob/cc65905efa5e2b7f2ba3d3c5ce0a69a716b95630/rclcpp/src/rclcpp/timer.cpp#L92

Documentation:

https://github.com/ros2/rcl/blob/ffcfda18db7aa9ad7c01ebdd26ae7bdd73c8206a/rcl/include/rcl/timer.h#L493-L519

rcl_timer_is_ready

https://github.com/ros2/rclcpp/blob/cc65905efa5e2b7f2ba3d3c5ce0a69a716b95630/rclcpp/src/rclcpp/timer.cpp#L111

Documentation:

https://github.com/ros2/rcl/blob/ffcfda18db7aa9ad7c01ebdd26ae7bdd73c8206a/rcl/include/rcl/timer.h#L262-L290

rcl_timer_get_time_until_next_call

https://github.com/ros2/rclcpp/blob/cc65905efa5e2b7f2ba3d3c5ce0a69a716b95630/rclcpp/src/rclcpp/timer.cpp#L122-124

Documentation:

https://github.com/ros2/rcl/blob/ffcfda18db7aa9ad7c01ebdd26ae7bdd73c8206a/rcl/include/rcl/timer.h#L292-L325

Returning timer_handle_

I don't believe anything should be done here.

https://github.com/ros2/rclcpp/blob/cc65905efa5e2b7f2ba3d3c5ce0a69a716b95630/rclcpp/src/rclcpp/timer.cpp#L136

timer_handle_ usages in timer.hpp

https://github.com/ros2/rclcpp/blob/cc65905efa5e2b7f2ba3d3c5ce0a69a716b95630/rclcpp/include/rclcpp/timer.hpp#L201

Documentation:

https://github.com/ros2/rcl/blob/ffcfda18db7aa9ad7c01ebdd26ae7bdd73c8206a/rcl/include/rcl/timer.h#L192-L234

[ivanpauno]

I believe that the timer class is thread safe because all of the functions that use timer_handle_ say that they are thread safe in their documentation, except for rcl_timer_init which is in the constructor and is locked with a mutex from the clock. Below are the functions and the documentation that states they are thread safe.

Thanks for your analysis, I believe it's correct.
I've also checked some of the methods marked as thread safe in rcl, and they seem to be so.

rcl_timer_init which is in the constructor and is locked with a mutex from the clock

I don't like much using the mutex of another object, though reading the implementation this is unfortunately needed (a refactor in rcl would be needed to change that).

I'm not sure why some of the code-blocks below aren't importing correctly.

Code blocks from other repositories aren't rendered.
An extra space/new-line might affect if a code block is rendered or not.

---

It seems that this was only a false alarm from my side, so I will close the issue.
Thanks @audrow for the detailed analysis.