Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions lib/timer.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,35 @@ class Timer {
return rclnodejs.getTimerPeriod(this._handle);
}

/**
* Set the on reset callback.
* @param {function} callback - The callback to be called when the timer is reset.
* @return {undefined}
*/
setOnResetCallback(callback) {
if (DistroUtils.getDistroId() <= DistroUtils.getDistroId('humble')) {
console.warn(
'setOnResetCallback is not supported by this version of ROS 2'
);
return;
}
rclnodejs.setTimerOnResetCallback(this._handle, callback);
}

/**
* Clear the on reset callback.
* @return {undefined}
*/
clearOnResetCallback() {
if (DistroUtils.getDistroId() <= DistroUtils.getDistroId('humble')) {
console.warn(
'clearOnResetCallback is not supported by this version of ROS 2'
);
return;
}
rclnodejs.clearTimerOnResetCallback(this._handle);
}

/**
* Call a timer and starts counting again, retrieves actual and expected call time.
* @return {object} - The timer information.
Expand Down
112 changes: 112 additions & 0 deletions src/rcl_timer_bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Comment on lines +38 to +60

Copilot AI Dec 11, 2025

Copy link

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.

Copilot uses AI. Check for mistakes.

Napi::Value CreateTimer(const Napi::CallbackInfo& info) {
Napi::Env env = info.Env();

Expand Down Expand Up @@ -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

Copilot AI Dec 11, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to rcl_timer_set_on_reset_callback should be guarded with a ROS_VERSION check since this function is only available in ROS versions after Humble. Without this guard, the code will fail to compile on older ROS versions. Consider adding an #if ROS_VERSION > 2205 guard around this call.

Suggested change
// 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 uses AI. Check for mistakes.

std::shared_ptr<TimerContext> context;
{
std::lock_guard<std::mutex> lock(g_timer_contexts_mutex);
auto it = g_timer_contexts.find(timer);
if (it != g_timer_contexts.end()) {
context = it->second;
g_timer_contexts.erase(it);
}
}
Comment on lines +108 to +115

Copilot AI Dec 11, 2025

Copy link

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 uses AI. Check for mistakes.

if (context) {
context->on_reset_callback.Release();
}
Comment on lines +107 to +119

Copilot AI Dec 11, 2025

Copy link

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 uses AI. Check for mistakes.

rcl_ret_t ret = rcl_timer_fini(timer);
free(ptr);
THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret,
Expand Down Expand Up @@ -215,6 +269,60 @@ Napi::Value CallTimerWithInfo(const Napi::CallbackInfo& info) {
Napi::BigInt::New(env, call_info.actual_call_time));
return timer_info;
}

Napi::Value SetTimerOnResetCallback(const Napi::CallbackInfo& info) {
Napi::Env env = info.Env();
RclHandle* timer_handle = RclHandle::Unwrap(info[0].As<Napi::Object>());
rcl_timer_t* timer = reinterpret_cast<rcl_timer_t*>(timer_handle->ptr());

if (!info[1].IsFunction()) {
Napi::TypeError::New(env, "Callback must be a function")
.ThrowAsJavaScriptException();
return env.Undefined();
}

Napi::Function callback = info[1].As<Napi::Function>();

std::lock_guard<std::mutex> lock(g_timer_contexts_mutex);
std::shared_ptr<TimerContext> context;
auto it = g_timer_contexts.find(timer);
if (it == g_timer_contexts.end()) {
context = std::make_shared<TimerContext>();
g_timer_contexts[timer] = context;
} else {
context = it->second;
context->on_reset_callback.Release();
}
Comment on lines +295 to +298

Copilot AI Dec 11, 2025

Copy link

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 uses AI. Check for mistakes.

context->on_reset_callback = Napi::ThreadSafeFunction::New(
env, callback, "TimerOnResetCallback", 0, 1);

THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK,
rcl_timer_set_on_reset_callback(
timer, TimerOnResetCallbackTrampoline, timer),
rcl_get_error_string().str);

return env.Undefined();
}

Napi::Value ClearTimerOnResetCallback(const Napi::CallbackInfo& info) {
Napi::Env env = info.Env();
RclHandle* timer_handle = RclHandle::Unwrap(info[0].As<Napi::Object>());
rcl_timer_t* timer = reinterpret_cast<rcl_timer_t*>(timer_handle->ptr());

std::lock_guard<std::mutex> lock(g_timer_contexts_mutex);
auto it = g_timer_contexts.find(timer);
if (it != g_timer_contexts.end()) {
it->second->on_reset_callback.Release();
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);

Comment on lines +319 to +326

Copilot AI Dec 11, 2025

Copy link

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.

Suggested change
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);
}

Copilot uses AI. Check for mistakes.
return env.Undefined();
}
#endif

Napi::Object InitTimerBindings(Napi::Env env, Napi::Object exports) {
Expand All @@ -231,6 +339,10 @@ Napi::Object InitTimerBindings(Napi::Env env, Napi::Object exports) {
exports.Set("changeTimerPeriod", Napi::Function::New(env, ChangeTimerPeriod));
exports.Set("getTimerPeriod", Napi::Function::New(env, GetTimerPeriod));
#if ROS_VERSION > 2205 // 2205 == Humble
exports.Set("setTimerOnResetCallback",
Napi::Function::New(env, SetTimerOnResetCallback));
exports.Set("clearTimerOnResetCallback",
Napi::Function::New(env, ClearTimerOnResetCallback));
exports.Set("callTimerWithInfo", Napi::Function::New(env, CallTimerWithInfo));
#endif
return exports;
Expand Down
51 changes: 51 additions & 0 deletions test/test-timer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copilot AI Dec 11, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The events parameter received in the callback is not validated or used in this test. Consider adding an assertion to verify that the events parameter contains an expected value (e.g., assert.strictEqual(typeof events, 'number') or assert.ok(events >= 0)) to ensure the callback receives valid data and to make the test more comprehensive.

Copilot uses AI. Check for mistakes.
timer.reset();

setTimeout(() => {
assert.ok(called);
timer.cancel();
done();
}, 100);

rclnodejs.spin(node);
});
Comment on lines +170 to +191

Copilot AI Dec 11, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These test cases should skip when running on ROS versions that don't support setOnResetCallback (Humble and earlier). Add a version check similar to the one used in the timer.callTimerWithInfo test at line 158-161 to prevent test failures on older ROS versions.

Copilot uses AI. Check for mistakes.

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

Copilot AI Dec 11, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The events parameter is defined but not validated in this test. While the test correctly verifies that the callback is not called, it would be more complete to validate the events parameter in the setOnResetCallback test (line 173-175) to ensure proper data is passed when the callback is invoked.

Copilot uses AI. Check for mistakes.
timer.clearOnResetCallback();
timer.reset();

setTimeout(() => {
assert.ok(!called);
timer.cancel();
done();
}, 100);

rclnodejs.spin(node);
});
Comment on lines +193 to +215

Copilot AI Dec 11, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test suite lacks coverage for calling setOnResetCallback multiple times on the same timer to replace an existing callback. This scenario is handled in the implementation (lines 274-277 of rcl_timer_bindings.cpp) but not tested. Consider adding a test that sets a callback, then replaces it with a different callback, and verifies that only the new callback is invoked when the timer is reset.

Copilot uses AI. Check for mistakes.
Comment on lines +193 to +215

Copilot AI Dec 11, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case should skip when running on ROS versions that don't support clearOnResetCallback (Humble and earlier). Add a version check similar to the one used in the timer.callTimerWithInfo test at line 158-161 to prevent test failures on older ROS versions.

Copilot uses AI. Check for mistakes.

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);
});
});
});
2 changes: 2 additions & 0 deletions test/types/index.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,8 @@ expectType<boolean>(timer.isCanceled());
expectType<void>(timer.cancel());
expectType<void>(timer.changeTimerPeriod(BigInt(100000)));
expectType<bigint>(timer.timerPeriod());
expectType<void>(timer.setOnResetCallback((_events: number) => {}));
expectType<void>(timer.clearOnResetCallback());
expectType<object>(timer.callTimerWithInfo());

// ---- Rate ----
Expand Down
11 changes: 11 additions & 0 deletions types/timer.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,17 @@ declare module 'rclnodejs' {
*/
timerPeriod(): bigint;

/**
* Set the on reset callback.
* @param callback - The callback to be called when the timer is reset.
*/
setOnResetCallback(callback: (events: number) => void): void;

/**
* Clear the on reset callback.
*/
clearOnResetCallback(): void;

/**
* Call a timer and starts counting again, retrieves actual and expected call time.
* @return - The timer information.
Expand Down
Loading