diff --git a/index.js b/index.js index 823fbf0f..d347c68a 100644 --- a/index.js +++ b/index.js @@ -387,23 +387,35 @@ let rcl = { rclnodejs.init(context.handle, argv, context._domainId); - if (_rosVersionChecked) { - // no further processing required - return; - } + try { + if (_rosVersionChecked) { + // no further processing required + return; + } - const version = await getCurrentGeneratorVersion(); - const forced = - version === null || compareVersions(version, generator.version(), '<'); - if (forced) { - debug( - 'The generator will begin to create JavaScript code from ROS IDL files...' - ); - } + const version = await getCurrentGeneratorVersion(); + const forced = + version === null || compareVersions(version, generator.version(), '<'); + if (forced) { + debug( + 'The generator will begin to create JavaScript code from ROS IDL files...' + ); + } - await generator.generateAll(forced); - // TODO determine if tsd generateAll() should be here - _rosVersionChecked = true; + await generator.generateAll(forced); + // TODO determine if tsd generateAll() should be here + _rosVersionChecked = true; + } catch (error) { + try { + context.tryShutdown(); + } catch (shutdownError) { + const initError = + error instanceof Error ? error : new Error(String(error)); + initError.message += ` (rollback also failed: ${shutdownError.message})`; + throw initError; + } + throw error; + } }, /** diff --git a/src/rcl_action_client_bindings.cpp b/src/rcl_action_client_bindings.cpp index f180563c..de548e34 100644 --- a/src/rcl_action_client_bindings.cpp +++ b/src/rcl_action_client_bindings.cpp @@ -132,7 +132,7 @@ Napi::Value ActionSendGoalRequest(const Napi::CallbackInfo& info) { rcl_action_send_goal_request(action_client, buffer, &sequence_number), RCL_RET_OK, rcl_get_error_string().str); - return Napi::Number::New(env, static_cast(sequence_number)); + return Napi::Number::New(env, static_cast(sequence_number)); } Napi::Value ActionSendResultRequest(const Napi::CallbackInfo& info) { @@ -149,7 +149,7 @@ Napi::Value ActionSendResultRequest(const Napi::CallbackInfo& info) { rcl_action_send_result_request(action_client, buffer, &sequence_number), RCL_RET_OK, rcl_get_error_string().str); - return Napi::Number::New(env, static_cast(sequence_number)); + return Napi::Number::New(env, static_cast(sequence_number)); } Napi::Value ActionTakeFeedback(const Napi::CallbackInfo& info) { @@ -163,9 +163,9 @@ Napi::Value ActionTakeFeedback(const Napi::CallbackInfo& info) { rcl_ret_t ret = rcl_action_take_feedback(action_client, buffer); if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) { - Napi::Error::New(env, rcl_get_error_string().str) - .ThrowAsJavaScriptException(); + std::string error_msg = rcl_get_error_string().str; rcl_reset_error(); + Napi::Error::New(env, error_msg).ThrowAsJavaScriptException(); return Napi::Boolean::New(env, false); } @@ -186,9 +186,9 @@ Napi::Value ActionTakeStatus(const Napi::CallbackInfo& info) { rcl_ret_t ret = rcl_action_take_status(action_client, buffer); if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) { + std::string error_msg = rcl_get_error_string().str; rcl_reset_error(); - Napi::Error::New(env, rcl_get_error_string().str) - .ThrowAsJavaScriptException(); + Napi::Error::New(env, error_msg).ThrowAsJavaScriptException(); return Napi::Boolean::New(env, false); } @@ -247,7 +247,7 @@ Napi::Value ActionSendCancelRequest(const Napi::CallbackInfo& info) { rcl_action_send_cancel_request(action_client, buffer, &sequence_number), RCL_RET_OK, rcl_get_error_string().str); - return Napi::Number::New(env, static_cast(sequence_number)); + return Napi::Number::New(env, static_cast(sequence_number)); } #if ROS_VERSION >= 2505 // ROS2 >= Kilted diff --git a/src/rcl_action_server_bindings.cpp b/src/rcl_action_server_bindings.cpp index cbf9efdf..977daf75 100644 --- a/src/rcl_action_server_bindings.cpp +++ b/src/rcl_action_server_bindings.cpp @@ -126,6 +126,7 @@ Napi::Value ActionTakeResultRequest(const Napi::CallbackInfo& info) { return js_obj; } + free(header); return env.Undefined(); } @@ -148,6 +149,7 @@ Napi::Value ActionTakeGoalRequest(const Napi::CallbackInfo& info) { return js_obj; } + free(header); return env.Undefined(); } @@ -221,14 +223,14 @@ Napi::Value ActionTakeGoalResponse(const Napi::CallbackInfo& info) { free(header); if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) { + std::string error_msg = rcl_get_error_string().str; rcl_reset_error(); - Napi::Error::New(env, rcl_get_error_string().str) - .ThrowAsJavaScriptException(); + Napi::Error::New(env, error_msg).ThrowAsJavaScriptException(); return env.Undefined(); } if (ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) { - return Napi::Number::New(env, static_cast(sequence_number)); + return Napi::Number::New(env, static_cast(sequence_number)); } return env.Undefined(); } @@ -250,14 +252,14 @@ Napi::Value ActionTakeCancelResponse(const Napi::CallbackInfo& info) { free(header); if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) { + std::string error_msg = rcl_get_error_string().str; rcl_reset_error(); - Napi::Error::New(env, rcl_get_error_string().str) - .ThrowAsJavaScriptException(); + Napi::Error::New(env, error_msg).ThrowAsJavaScriptException(); return env.Undefined(); } if (ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) { - return Napi::Number::New(env, static_cast(sequence_number)); + return Napi::Number::New(env, static_cast(sequence_number)); } return env.Undefined(); } @@ -279,14 +281,14 @@ Napi::Value ActionTakeResultResponse(const Napi::CallbackInfo& info) { free(header); if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) { + std::string error_msg = rcl_get_error_string().str; rcl_reset_error(); - Napi::Error::New(env, rcl_get_error_string().str) - .ThrowAsJavaScriptException(); + Napi::Error::New(env, error_msg).ThrowAsJavaScriptException(); return env.Undefined(); } if (ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) { - return Napi::Number::New(env, static_cast(sequence_number)); + return Napi::Number::New(env, static_cast(sequence_number)); } return env.Undefined(); } @@ -463,6 +465,7 @@ Napi::Value ActionTakeCancelRequest(const Napi::CallbackInfo& info) { return js_obj; } + free(header); return env.Undefined(); } diff --git a/src/rcl_client_bindings.cpp b/src/rcl_client_bindings.cpp index ee7ad0f8..1d820c3c 100644 --- a/src/rcl_client_bindings.cpp +++ b/src/rcl_client_bindings.cpp @@ -88,7 +88,7 @@ Napi::Value SendRequest(const Napi::CallbackInfo& info) { THROW_ERROR_IF_NOT_EQUAL(rcl_send_request(client, buffer, &sequence_number), RCL_RET_OK, rcl_get_error_string().str); - return Napi::Number::New(env, static_cast(sequence_number)); + return Napi::Number::New(env, static_cast(sequence_number)); } Napi::Value RclTakeResponse(const Napi::CallbackInfo& info) { @@ -104,7 +104,7 @@ Napi::Value RclTakeResponse(const Napi::CallbackInfo& info) { int64_t sequence_number = header.request_id.sequence_number; if (ret == RCL_RET_OK) { - return Napi::Number::New(env, static_cast(sequence_number)); + return Napi::Number::New(env, static_cast(sequence_number)); } rcl_reset_error(); diff --git a/src/rcl_service_bindings.cpp b/src/rcl_service_bindings.cpp index 09e15a9d..c759c9b1 100644 --- a/src/rcl_service_bindings.cpp +++ b/src/rcl_service_bindings.cpp @@ -96,6 +96,7 @@ Napi::Value RclTakeRequest(const Napi::CallbackInfo& info) { return js_obj; } + free(header); return env.Undefined(); } diff --git a/src/rcl_subscription_bindings.cpp b/src/rcl_subscription_bindings.cpp index 2f5ee251..49b779ce 100644 --- a/src/rcl_subscription_bindings.cpp +++ b/src/rcl_subscription_bindings.cpp @@ -19,6 +19,8 @@ #include #include +#include +// NOLINTNEXTLINE #include #include "macros.h" @@ -108,12 +110,6 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) { rcl_ret_t ret = rcl_subscription_options_set_content_filter_options( expression.c_str(), argc, (const char**)argv, &subscription_ops); - if (ret != RCL_RET_OK) { - std::string error_string = rcl_get_error_string().str; - rcl_reset_error(); - Napi::Error::New(env, error_string).ThrowAsJavaScriptException(); - } - if (argc) { for (int i = 0; i < argc; i++) { free(argv[i]); @@ -122,7 +118,10 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) { } if (ret != RCL_RET_OK) { + std::string error_string = rcl_get_error_string().str; + rcl_reset_error(); free(subscription); + Napi::Error::New(env, error_string).ThrowAsJavaScriptException(); return env.Undefined(); } } @@ -137,8 +136,8 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) { if (ret != RCL_RET_OK) { std::string error_msg = rcl_get_error_string().str; rcl_reset_error(); - Napi::Error::New(env, error_msg).ThrowAsJavaScriptException(); free(subscription); + Napi::Error::New(env, error_msg).ThrowAsJavaScriptException(); return env.Undefined(); } @@ -157,9 +156,9 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) { return js_obj; } else { - Napi::Error::New(env, GetErrorMessageAndClear()) - .ThrowAsJavaScriptException(); + std::string error_msg = GetErrorMessageAndClear(); free(subscription); + Napi::Error::New(env, error_msg).ThrowAsJavaScriptException(); return env.Undefined(); } } @@ -194,10 +193,16 @@ Napi::Value RclTakeRaw(const Napi::CallbackInfo& info) { return env.Undefined(); } + RCPPUTILS_SCOPE_EXIT({ + rcl_ret_t fini_ret = rmw_serialized_message_fini(&msg); + if (fini_ret != RCL_RET_OK) { + rcl_reset_error(); + } + }); + Napi::Buffer buffer = Napi::Buffer::Copy( env, reinterpret_cast(msg.buffer), msg.buffer_length); - THROW_ERROR_IF_NOT_EQUAL(rmw_serialized_message_fini(&msg), RCL_RET_OK, - "Failed to deallocate message buffer"); + return buffer; } @@ -369,6 +374,14 @@ Napi::Value GetContentFilter(const Napi::CallbackInfo& info) { return env.Undefined(); } + RCPPUTILS_SCOPE_EXIT({ + rcl_ret_t fini_ret = + rcl_subscription_content_filter_options_fini(subscription, &options); + if (fini_ret != RCL_RET_OK) { + rcl_reset_error(); + } + }); + // Create result object Napi::Object result = Napi::Object::New(env); result.Set( @@ -387,16 +400,6 @@ Napi::Value GetContentFilter(const Napi::CallbackInfo& info) { } result.Set("parameters", parameters); - // Cleanup - rcl_ret_t fini_ret = - rcl_subscription_content_filter_options_fini(subscription, &options); - if (fini_ret != RCL_RET_OK) { - std::string error_msg = rcl_get_error_string().str; - rcl_reset_error(); - Napi::Error::New(env, error_msg).ThrowAsJavaScriptException(); - return env.Undefined(); - } - return result; } diff --git a/test/test-init-shutdown.js b/test/test-init-shutdown.js index ac837104..94071dc3 100644 --- a/test/test-init-shutdown.js +++ b/test/test-init-shutdown.js @@ -15,7 +15,9 @@ 'use strict'; const assert = require('assert'); +const sinon = require('sinon'); const rclnodejs = require('../index.js'); +const generator = require('../rosidl_gen/index.js'); describe('rclnodejs init and shutdown test suite', function () { this.timeout(60 * 1000); @@ -99,6 +101,40 @@ describe('rclnodejs init and shutdown test suite', function () { }, 'shutting rclnodejs down twice should not cause an error to be thrown'); }); + it('rolls back context when generator init step fails', async function () { + rclnodejs.shutdownAll(); + + const generateAllStub = sinon + .stub(generator, 'generateAll') + .rejects(new Error('generator failed')); + + try { + const indexPath = require.resolve('../index.js'); + delete require.cache[indexPath]; + const freshRclnodejs = require('../index.js'); + + const failedContext = new freshRclnodejs.Context(); + + await assert.rejects(async () => { + await freshRclnodejs.init(failedContext); + }, /generator failed/); + assert.ok(freshRclnodejs.isShutdown(failedContext)); + + generateAllStub.restore(); + + const recoveredContext = new freshRclnodejs.Context(); + await assert.doesNotReject(async () => { + await freshRclnodejs.init(recoveredContext); + }); + freshRclnodejs.shutdown(recoveredContext); + freshRclnodejs.removeSignalHandlers(); + } finally { + if (generateAllStub.restore) { + generateAllStub.restore(); + } + } + }); + it('rclnodejs create node without init should fail', async function () { assert.throws( () => {