-
Notifications
You must be signed in to change notification settings - Fork 85
Add rcl_timer_set_on_reset_callback() support #1348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,12 +17,47 @@ | |||||||||||||||||||||||||||||||||
| #include <rcl/error_handling.h> | ||||||||||||||||||||||||||||||||||
| #include <rcl/rcl.h> | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| #include <mutex> | ||||||||||||||||||||||||||||||||||
| #include <unordered_map> | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| #include "macros.h" | ||||||||||||||||||||||||||||||||||
| #include "rcl_handle.h" | ||||||||||||||||||||||||||||||||||
| #include "rcl_utilities.h" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| namespace rclnodejs { | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| struct TimerContext { | ||||||||||||||||||||||||||||||||||
| Napi::ThreadSafeFunction on_reset_callback; | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| static std::unordered_map<rcl_timer_t*, std::shared_ptr<TimerContext>> | ||||||||||||||||||||||||||||||||||
| g_timer_contexts; | ||||||||||||||||||||||||||||||||||
| static std::mutex g_timer_contexts_mutex; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| void TimerOnResetCallbackTrampoline(const void* user_data, | ||||||||||||||||||||||||||||||||||
| size_t number_of_events) { | ||||||||||||||||||||||||||||||||||
| const rcl_timer_t* timer = static_cast<const rcl_timer_t*>(user_data); | ||||||||||||||||||||||||||||||||||
| std::shared_ptr<TimerContext> context; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| std::lock_guard<std::mutex> lock(g_timer_contexts_mutex); | ||||||||||||||||||||||||||||||||||
| auto it = g_timer_contexts.find(const_cast<rcl_timer_t*>(timer)); | ||||||||||||||||||||||||||||||||||
| if (it != g_timer_contexts.end()) { | ||||||||||||||||||||||||||||||||||
| context = it->second; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if (context) { | ||||||||||||||||||||||||||||||||||
| auto callback = [](Napi::Env env, Napi::Function js_callback, | ||||||||||||||||||||||||||||||||||
| size_t* events) { | ||||||||||||||||||||||||||||||||||
| js_callback.Call({Napi::Number::New(env, *events)}); | ||||||||||||||||||||||||||||||||||
| delete events; | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
| size_t* events_ptr = new size_t(number_of_events); | ||||||||||||||||||||||||||||||||||
| context->on_reset_callback.BlockingCall(events_ptr, callback); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Napi::Value CreateTimer(const Napi::CallbackInfo& info) { | ||||||||||||||||||||||||||||||||||
| Napi::Env env = info.Env(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -61,6 +96,25 @@ Napi::Value CreateTimer(const Napi::CallbackInfo& info) { | |||||||||||||||||||||||||||||||||
| auto js_obj = | ||||||||||||||||||||||||||||||||||
| RclHandle::NewInstance(env, timer, clock_handle, [env](void* ptr) { | ||||||||||||||||||||||||||||||||||
| rcl_timer_t* timer = reinterpret_cast<rcl_timer_t*>(ptr); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // Clear the callback first to prevent any new callbacks from being | ||||||||||||||||||||||||||||||||||
| // triggered | ||||||||||||||||||||||||||||||||||
| rcl_timer_set_on_reset_callback(timer, nullptr, nullptr); | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+103
to
+104
|
||||||||||||||||||||||||||||||||||
| // triggered | |
| rcl_timer_set_on_reset_callback(timer, nullptr, nullptr); | |
| // triggered | |
| #if ROS_VERSION > 2205 | |
| rcl_timer_set_on_reset_callback(timer, nullptr, nullptr); | |
| #endif |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a potential race condition between the timer cleanup in the destructor and concurrent access from TimerOnResetCallbackTrampoline. While the mutex is held during cleanup, the trampoline function accesses the context without the mutex. If a reset callback is triggered just before or during timer destruction, the context could be deleted while the trampoline is accessing it. Consider clearing the callback in rcl_timer_set_on_reset_callback before releasing the context to ensure no callbacks are in flight.
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanup code for the timer context should be guarded with a ROS_VERSION check since rcl_timer_set_on_reset_callback and the associated context management are only available in ROS versions after Humble. This entire block should be wrapped in #if ROS_VERSION > 2205 to prevent compilation errors on older ROS versions.
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When replacing an existing callback, Release() is called on the old ThreadSafeFunction while still holding the mutex. However, if a timer reset event is currently being processed by TimerOnResetCallbackTrampoline, this could lead to a race condition. The old callback's Release() might complete while the trampoline is still trying to use it. Consider ensuring that rcl_timer_set_on_reset_callback with the new callback is called before releasing the old ThreadSafeFunction, or implement proper synchronization to prevent the trampoline from accessing a callback that's being released.
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the timer destructor, this function has a race condition. The context is released and deleted while holding the mutex, but TimerOnResetCallbackTrampoline could be executing concurrently without the mutex. The rcl_timer_set_on_reset_callback call happens after the context is already deleted, so if a reset event was triggered just before this function was called, the trampoline could access deleted memory. Consider calling rcl_timer_set_on_reset_callback to clear the callback before releasing and deleting the context.
| it->second->on_reset_callback.Release(); | |
| delete it->second; | |
| g_timer_contexts.erase(it); | |
| } | |
| THROW_ERROR_IF_NOT_EQUAL( | |
| RCL_RET_OK, rcl_timer_set_on_reset_callback(timer, nullptr, nullptr), | |
| rcl_get_error_string().str); | |
| // Clear the callback before releasing and deleting the context to avoid race condition | |
| THROW_ERROR_IF_NOT_EQUAL( | |
| RCL_RET_OK, rcl_timer_set_on_reset_callback(timer, nullptr, nullptr), | |
| rcl_get_error_string().str); | |
| it->second->on_reset_callback.Release(); | |
| delete it->second; | |
| g_timer_contexts.erase(it); | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,5 +166,56 @@ describe('rclnodejs Timer class testing', function () { | |
| timer.cancel(); | ||
| done(); | ||
| }); | ||
|
|
||
| it('timer.setOnResetCallback', function (done) { | ||
| var timer = node.createTimer(TIMER_INTERVAL, function () {}); | ||
| var called = false; | ||
| timer.setOnResetCallback(function (events) { | ||
| assert.strictEqual(typeof events, 'number'); | ||
| assert.ok(events >= 0); | ||
| called = true; | ||
| }); | ||
|
Comment on lines
+177
to
+181
|
||
| timer.reset(); | ||
|
|
||
| setTimeout(() => { | ||
| assert.ok(called); | ||
| timer.cancel(); | ||
| done(); | ||
| }, 100); | ||
|
|
||
| rclnodejs.spin(node); | ||
| }); | ||
|
Comment on lines
+170
to
+191
|
||
|
|
||
| it('timer.clearOnResetCallback', function (done) { | ||
| var timer = node.createTimer(TIMER_INTERVAL, function () {}); | ||
| var called = false; | ||
| timer.setOnResetCallback(function (events) { | ||
| assert.strictEqual(typeof events, 'number'); | ||
| assert.ok(events >= 0); | ||
| called = true; | ||
| }); | ||
|
Comment on lines
+200
to
+204
|
||
| timer.clearOnResetCallback(); | ||
| timer.reset(); | ||
|
|
||
| setTimeout(() => { | ||
| assert.ok(!called); | ||
| timer.cancel(); | ||
| done(); | ||
| }, 100); | ||
|
|
||
| rclnodejs.spin(node); | ||
| }); | ||
|
Comment on lines
+193
to
+215
|
||
|
|
||
| it('timer callback should be called repeatedly', function (done) { | ||
| let count = 0; | ||
| const timer = node.createTimer(TIMER_INTERVAL, () => { | ||
| count++; | ||
| if (count >= 3) { | ||
| timer.cancel(); | ||
| done(); | ||
| } | ||
| }); | ||
| rclnodejs.spin(node); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a potential race condition where the TimerContext could be deleted while TimerOnResetCallbackTrampoline is executing. The trampoline accesses the context pointer without holding g_timer_contexts_mutex, but ClearTimerOnResetCallback or the timer destructor could delete the context concurrently. This could lead to a use-after-free error. Consider using a shared_ptr for TimerContext or implementing reference counting to ensure the context remains valid during callback execution.