From 9c29dca894956da34ab932201cf7ef86eb562bc0 Mon Sep 17 00:00:00 2001 From: Minggang Wang Date: Mon, 23 Jun 2025 17:31:26 +0800 Subject: [PATCH 1/2] Ensure binding method returns with undefined when error happens --- src/macros.h | 16 ++++++++++++++++ src/rcl_action_client_bindings.cpp | 5 +++-- src/rcl_action_goal_bindings.cpp | 5 +++-- src/rcl_action_server_bindings.cpp | 12 +++++++----- src/rcl_client_bindings.cpp | 7 ++++--- src/rcl_context_bindings.cpp | 3 ++- src/rcl_guard_condition_bindings.cpp | 5 +++-- src/rcl_lifecycle_bindings.cpp | 10 ++++++---- src/rcl_node_bindings.cpp | 5 +++-- src/rcl_publisher_bindings.cpp | 7 ++++--- src/rcl_service_bindings.cpp | 7 ++++--- src/rcl_subscription_bindings.cpp | 5 +++-- src/rcl_time_point_bindings.cpp | 5 +++-- src/rcl_timer_bindings.cpp | 14 ++++++++------ 14 files changed, 69 insertions(+), 37 deletions(-) diff --git a/src/macros.h b/src/macros.h index 793aa41b..c3eea1bd 100644 --- a/src/macros.h +++ b/src/macros.h @@ -25,15 +25,31 @@ rcl_reset_error(); \ Napi::Error::New(rclnodejs::GetEnv(), message) \ .ThrowAsJavaScriptException(); \ + return env.Undefined(); \ } \ } +#define CHECK_OP_AND_THROW_ERROR_IF_NOT_TRUE_NO_RETURN(op, lhs, rhs, message) \ + { \ + if (lhs op rhs) { \ + rcl_reset_error(); \ + Napi::Error::New(rclnodejs::GetEnv(), message) \ + .ThrowAsJavaScriptException(); \ + } \ + } + #define THROW_ERROR_IF_NOT_EQUAL(lhs, rhs, message) \ CHECK_OP_AND_THROW_ERROR_IF_NOT_TRUE(!=, lhs, rhs, message) #define THROW_ERROR_IF_EQUAL(lhs, rhs, message) \ CHECK_OP_AND_THROW_ERROR_IF_NOT_TRUE(==, lhs, rhs, message) +#define THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(lhs, rhs, message) \ + CHECK_OP_AND_THROW_ERROR_IF_NOT_TRUE_NO_RETURN(!=, lhs, rhs, message) + +#define THROW_ERROR_IF_EQUAL_NO_RETURN(lhs, rhs, message) \ + CHECK_OP_AND_THROW_ERROR_IF_NOT_TRUE_NO_RETURN(==, lhs, rhs, message) + #define PACKAGE_NAME "rclnodejs" #ifdef DEBUG_ON diff --git a/src/rcl_action_client_bindings.cpp b/src/rcl_action_client_bindings.cpp index 8a45f13d..738a39d4 100644 --- a/src/rcl_action_client_bindings.cpp +++ b/src/rcl_action_client_bindings.cpp @@ -75,12 +75,13 @@ Napi::Value ActionCreateClient(const Napi::CallbackInfo& info) { &action_client_ops), RCL_RET_OK, rcl_get_error_string().str); auto js_obj = RclHandle::NewInstance( - env, action_client, node_handle, [node](void* ptr) { + env, action_client, node_handle, [node, env](void* ptr) { rcl_action_client_t* action_client = reinterpret_cast(ptr); rcl_ret_t ret = rcl_action_client_fini(action_client, node); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return js_obj; diff --git a/src/rcl_action_goal_bindings.cpp b/src/rcl_action_goal_bindings.cpp index 2a379c3e..6c3a64fe 100644 --- a/src/rcl_action_goal_bindings.cpp +++ b/src/rcl_action_goal_bindings.cpp @@ -49,12 +49,13 @@ Napi::Value ActionAcceptNewGoal(const Napi::CallbackInfo& info) { malloc(sizeof(rcl_action_goal_handle_t))); *goal_handle = *new_goal; auto js_obj = - RclHandle::NewInstance(env, goal_handle, nullptr, [](void* ptr) { + RclHandle::NewInstance(env, goal_handle, nullptr, [env](void* ptr) { rcl_action_goal_handle_t* goal_handle = reinterpret_cast(ptr); rcl_ret_t ret = rcl_action_goal_handle_fini(goal_handle); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return js_obj; diff --git a/src/rcl_action_server_bindings.cpp b/src/rcl_action_server_bindings.cpp index b4aa2b14..f6abdbf0 100644 --- a/src/rcl_action_server_bindings.cpp +++ b/src/rcl_action_server_bindings.cpp @@ -81,12 +81,13 @@ Napi::Value ActionCreateServer(const Napi::CallbackInfo& info) { action_name.c_str(), &action_server_ops), RCL_RET_OK, rcl_get_error_string().str); auto js_obj = RclHandle::NewInstance( - env, action_server, node_handle, [node](void* ptr) { + env, action_server, node_handle, [node, env](void* ptr) { rcl_action_server_t* action_server = reinterpret_cast(ptr); rcl_ret_t ret = rcl_action_server_fini(action_server, node); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return js_obj; @@ -390,13 +391,14 @@ Napi::Value ActionProcessCancelRequest(const Napi::CallbackInfo& info) { } *response = cancel_response_ptr->msg; - auto js_obj = - RclHandle::NewInstance(env, cancel_response_ptr, nullptr, [](void* ptr) { + auto js_obj = RclHandle::NewInstance( + env, cancel_response_ptr, nullptr, [env](void* ptr) { rcl_action_cancel_response_t* cancel_response_ptr = reinterpret_cast(ptr); rcl_ret_t ret = rcl_action_cancel_response_fini(cancel_response_ptr); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return js_obj; } diff --git a/src/rcl_client_bindings.cpp b/src/rcl_client_bindings.cpp index a51ed9f7..d910df8f 100644 --- a/src/rcl_client_bindings.cpp +++ b/src/rcl_client_bindings.cpp @@ -53,12 +53,13 @@ Napi::Value CreateClient(const Napi::CallbackInfo& info) { rcl_client_init(client, node, ts, service_name.c_str(), &client_ops), RCL_RET_OK, rcl_get_error_string().str); - auto js_obj = - RclHandle::NewInstance(env, client, node_handle, [node](void* ptr) { + auto js_obj = RclHandle::NewInstance( + env, client, node_handle, [node, env](void* ptr) { rcl_client_t* client = reinterpret_cast(ptr); rcl_ret_t ret = rcl_client_fini(client, node); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return js_obj; diff --git a/src/rcl_context_bindings.cpp b/src/rcl_context_bindings.cpp index 40dac48b..4a75b305 100644 --- a/src/rcl_context_bindings.cpp +++ b/src/rcl_context_bindings.cpp @@ -120,7 +120,8 @@ Napi::Value CreateContext(const Napi::CallbackInfo& info) { rcl_context_t* context = reinterpret_cast(ptr); rcl_ret_t ret = DestroyContext(env, context); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return js_obj; diff --git a/src/rcl_guard_condition_bindings.cpp b/src/rcl_guard_condition_bindings.cpp index ffb34e9e..ba3475e2 100644 --- a/src/rcl_guard_condition_bindings.cpp +++ b/src/rcl_guard_condition_bindings.cpp @@ -41,11 +41,12 @@ Napi::Value CreateGuardCondition(const Napi::CallbackInfo& info) { rcl_guard_condition_init(gc, context, gc_options), rcl_get_error_string().str); - auto handle = RclHandle::NewInstance(env, gc, nullptr, [](void* ptr) { + auto handle = RclHandle::NewInstance(env, gc, nullptr, [env](void* ptr) { rcl_guard_condition_t* gc = reinterpret_cast(ptr); rcl_ret_t ret = rcl_guard_condition_fini(gc); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return handle; diff --git a/src/rcl_lifecycle_bindings.cpp b/src/rcl_lifecycle_bindings.cpp index 702aaf28..9122cb1c 100644 --- a/src/rcl_lifecycle_bindings.cpp +++ b/src/rcl_lifecycle_bindings.cpp @@ -83,12 +83,13 @@ Napi::Value CreateLifecycleStateMachine(const Napi::CallbackInfo& info) { rcl_get_error_string().str); auto js_obj = RclHandle::NewInstance( - env, state_machine, node_handle, [node](void* ptr) { + env, state_machine, node_handle, [node, env](void* ptr) { rcl_lifecycle_state_machine_t* state_machine = reinterpret_cast(ptr); rcl_ret_t ret = rcl_lifecycle_state_machine_fini(state_machine, node); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); #else const rcl_node_options_t* node_options = @@ -101,13 +102,14 @@ Napi::Value CreateLifecycleStateMachine(const Napi::CallbackInfo& info) { rcl_get_error_string().str); auto js_obj = RclHandle::NewInstance( - env, state_machine, node_handle, [node, node_options](void* ptr) { + env, state_machine, node_handle, [node, node_options, env](void* ptr) { rcl_lifecycle_state_machine_t* state_machine = reinterpret_cast(ptr); rcl_ret_t ret = rcl_lifecycle_state_machine_fini( state_machine, node, &node_options->allocator); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); #endif diff --git a/src/rcl_node_bindings.cpp b/src/rcl_node_bindings.cpp index 072e1dab..2c647ddb 100644 --- a/src/rcl_node_bindings.cpp +++ b/src/rcl_node_bindings.cpp @@ -217,11 +217,12 @@ Napi::Value CreateNode(const Napi::CallbackInfo& info) { name_space.c_str(), context, &options), rcl_get_error_string().str); - auto handle = RclHandle::NewInstance(env, node, nullptr, [](void* ptr) { + auto handle = RclHandle::NewInstance(env, node, nullptr, [env](void* ptr) { rcl_node_t* node = reinterpret_cast(ptr); rcl_ret_t ret = rcl_node_fini(node); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return handle; diff --git a/src/rcl_publisher_bindings.cpp b/src/rcl_publisher_bindings.cpp index 35dd5df1..d138a4c5 100644 --- a/src/rcl_publisher_bindings.cpp +++ b/src/rcl_publisher_bindings.cpp @@ -55,12 +55,13 @@ Napi::Value CreatePublisher(const Napi::CallbackInfo& info) { rcl_publisher_init(publisher, node, ts, topic.c_str(), &publisher_ops), RCL_RET_OK, rcl_get_error_string().str); - auto js_obj = - RclHandle::NewInstance(env, publisher, node_handle, [node](void* ptr) { + auto js_obj = RclHandle::NewInstance( + env, publisher, node_handle, [node, env](void* ptr) { rcl_publisher_t* publisher = reinterpret_cast(ptr); rcl_ret_t ret = rcl_publisher_fini(publisher, node); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return js_obj; diff --git a/src/rcl_service_bindings.cpp b/src/rcl_service_bindings.cpp index d353ea98..2142d31d 100644 --- a/src/rcl_service_bindings.cpp +++ b/src/rcl_service_bindings.cpp @@ -55,12 +55,13 @@ Napi::Value CreateService(const Napi::CallbackInfo& info) { THROW_ERROR_IF_NOT_EQUAL( rcl_service_init(service, node, ts, service_name.c_str(), &service_ops), RCL_RET_OK, rcl_get_error_string().str); - auto js_obj = - RclHandle::NewInstance(env, service, node_handle, [node](void* ptr) { + auto js_obj = RclHandle::NewInstance( + env, service, node_handle, [node, env](void* ptr) { rcl_service_t* service = reinterpret_cast(ptr); rcl_ret_t ret = rcl_service_fini(service, node); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return js_obj; diff --git a/src/rcl_subscription_bindings.cpp b/src/rcl_subscription_bindings.cpp index bece3ea7..c8699a10 100644 --- a/src/rcl_subscription_bindings.cpp +++ b/src/rcl_subscription_bindings.cpp @@ -136,12 +136,13 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) { rcl_get_error_string().str); auto js_obj = RclHandle::NewInstance( - env, subscription, node_handle, [node](void* ptr) { + env, subscription, node_handle, [node, env](void* ptr) { rcl_subscription_t* subscription = reinterpret_cast(ptr); rcl_ret_t ret = rcl_subscription_fini(subscription, node); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); return js_obj; diff --git a/src/rcl_time_point_bindings.cpp b/src/rcl_time_point_bindings.cpp index d431a265..b73334c6 100644 --- a/src/rcl_time_point_bindings.cpp +++ b/src/rcl_time_point_bindings.cpp @@ -151,11 +151,12 @@ Napi::Value CreateClock(const Napi::CallbackInfo& info) { rcl_clock_init(clock_type, clock, &allocator), rcl_get_error_string().str); - return RclHandle::NewInstance(env, clock, nullptr, [](void* ptr) { + return RclHandle::NewInstance(env, clock, nullptr, [env](void* ptr) { rcl_clock_t* clock = reinterpret_cast(ptr); rcl_ret_t ret = rcl_clock_fini(clock); free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); }); } diff --git a/src/rcl_timer_bindings.cpp b/src/rcl_timer_bindings.cpp index 799adcd0..16341d6e 100644 --- a/src/rcl_timer_bindings.cpp +++ b/src/rcl_timer_bindings.cpp @@ -58,12 +58,14 @@ Napi::Value CreateTimer(const Napi::CallbackInfo& info) { rcl_get_error_string().str); #endif - auto js_obj = RclHandle::NewInstance(env, timer, clock_handle, [](void* ptr) { - rcl_timer_t* timer = reinterpret_cast(ptr); - rcl_ret_t ret = rcl_timer_fini(timer); - free(ptr); - THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, ret, rcl_get_error_string().str); - }); + auto js_obj = + RclHandle::NewInstance(env, timer, clock_handle, [env](void* ptr) { + rcl_timer_t* timer = reinterpret_cast(ptr); + rcl_ret_t ret = rcl_timer_fini(timer); + free(ptr); + THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret, + rcl_get_error_string().str); + }); return js_obj; } From 96b992278c59976deb478b7b734ec779d24764d7 Mon Sep 17 00:00:00 2001 From: Minggang Wang Date: Mon, 23 Jun 2025 17:58:16 +0800 Subject: [PATCH 2/2] Address comments --- src/macros.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/macros.h b/src/macros.h index c3eea1bd..846cfd15 100644 --- a/src/macros.h +++ b/src/macros.h @@ -23,7 +23,7 @@ { \ if (lhs op rhs) { \ rcl_reset_error(); \ - Napi::Error::New(rclnodejs::GetEnv(), message) \ + Napi::Error::New(env, message) \ .ThrowAsJavaScriptException(); \ return env.Undefined(); \ } \ @@ -33,7 +33,7 @@ { \ if (lhs op rhs) { \ rcl_reset_error(); \ - Napi::Error::New(rclnodejs::GetEnv(), message) \ + Napi::Error::New(env, message) \ .ThrowAsJavaScriptException(); \ } \ }