Skip to content

Commit faaa2d4

Browse files
authored
Support logging mutex functions (#1338)
This PR adds support for logging mutex functions to enable thread-safe logging configuration and shutdown. The changes introduce new `configure()` and `shutdown()` methods to the logging system with proper mutex protection. - Implements thread-safe logging output handler with recursive mutex - Adds static methods `configure()` and `shutdown()` to the `Logging` class - Includes TypeScript type definitions and comprehensive tests Fix: #1330
1 parent fb4597a commit faaa2d4

5 files changed

Lines changed: 128 additions & 0 deletions

File tree

lib/logging.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
const path = require('path');
1818
const rclnodejs = require('./native_loader.js');
1919
const { TypeValidationError } = require('./errors.js');
20+
const Context = require('./context.js');
2021

2122
/**
2223
* Enum for LoggingSeverity
@@ -264,6 +265,28 @@ class Logging {
264265
return new Logging(name);
265266
}
266267

268+
/**
269+
* Configure the logging system with the given context.
270+
* @param {Context} context - The context to configure logging for.
271+
* @function
272+
* @return {undefined}
273+
*/
274+
static configure(context) {
275+
if (!(context instanceof Context)) {
276+
throw new TypeValidationError('context', context, 'Context');
277+
}
278+
rclnodejs.loggingConfigure(context.handle);
279+
}
280+
281+
/**
282+
* Shutdown the logging system.
283+
* @function
284+
* @return {undefined}
285+
*/
286+
static shutdown() {
287+
rclnodejs.loggingFini();
288+
}
289+
267290
/**
268291
* Get the logging directory.
269292
* @function

src/rcl_logging_bindings.cpp

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include <rcl/rcl.h>
2121
#include <rcl_logging_interface/rcl_logging_interface.h>
2222

23+
#include <mutex>
2324
#include <string>
2425

2526
#include "macros.h"
@@ -40,6 +41,47 @@ Napi::Value InitRosoutPublisherForNode(const Napi::CallbackInfo& info) {
4041
.ThrowAsJavaScriptException();
4142
rcl_reset_error();
4243
}
44+
}
45+
return env.Undefined();
46+
}
47+
48+
static std::recursive_mutex logging_mutex;
49+
50+
static void rclnodejs_thread_safe_logging_output_handler(
51+
const rcutils_log_location_t* location, int severity, const char* name,
52+
rcutils_time_point_value_t timestamp, const char* format, va_list* args) {
53+
try {
54+
std::lock_guard<std::recursive_mutex> lock(logging_mutex);
55+
rcl_logging_multiple_output_handler(location, severity, name, timestamp,
56+
format, args);
57+
} catch (const std::exception& ex) {
58+
RCUTILS_SAFE_FWRITE_TO_STDERR(
59+
"rclnodejs failed to get the global logging mutex: ");
60+
RCUTILS_SAFE_FWRITE_TO_STDERR(ex.what());
61+
RCUTILS_SAFE_FWRITE_TO_STDERR("\n");
62+
} catch (...) {
63+
RCUTILS_SAFE_FWRITE_TO_STDERR(
64+
"rclnodejs failed to get the global logging mutex\n");
65+
}
66+
}
67+
68+
Napi::Value LoggingConfigure(const Napi::CallbackInfo& info) {
69+
Napi::Env env = info.Env();
70+
RclHandle* context_handle = RclHandle::Unwrap(info[0].As<Napi::Object>());
71+
rcl_context_t* context =
72+
reinterpret_cast<rcl_context_t*>(context_handle->ptr());
73+
74+
rcl_allocator_t allocator = rcl_get_default_allocator();
75+
76+
std::lock_guard<std::recursive_mutex> lock(logging_mutex);
77+
rcl_ret_t ret = rcl_logging_configure_with_output_handler(
78+
&context->global_arguments, &allocator,
79+
rclnodejs_thread_safe_logging_output_handler);
80+
81+
if (ret != RCL_RET_OK) {
82+
Napi::Error::New(env, rcl_get_error_string().str)
83+
.ThrowAsJavaScriptException();
84+
rcl_reset_error();
4385
}
4486
return env.Undefined();
4587
}
@@ -60,6 +102,18 @@ Napi::Value FiniRosoutPublisherForNode(const Napi::CallbackInfo& info) {
60102
return env.Undefined();
61103
}
62104

105+
Napi::Value LoggingFini(const Napi::CallbackInfo& info) {
106+
Napi::Env env = info.Env();
107+
std::lock_guard<std::recursive_mutex> lock(logging_mutex);
108+
rcl_ret_t ret = rcl_logging_fini();
109+
if (ret != RCL_RET_OK) {
110+
Napi::Error::New(env, rcl_get_error_string().str)
111+
.ThrowAsJavaScriptException();
112+
rcl_reset_error();
113+
}
114+
return env.Undefined();
115+
}
116+
63117
Napi::Value setLoggerLevel(const Napi::CallbackInfo& info) {
64118
Napi::Env env = info.Env();
65119

@@ -192,6 +246,8 @@ Napi::Object InitLoggingBindings(Napi::Env env, Napi::Object exports) {
192246
exports.Set("removeRosoutSublogger",
193247
Napi::Function::New(env, RemoveRosoutSublogger));
194248
#endif
249+
exports.Set("loggingConfigure", Napi::Function::New(env, LoggingConfigure));
250+
exports.Set("loggingFini", Napi::Function::New(env, LoggingFini));
195251
return exports;
196252
}
197253

test/test-logging.js

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ describe('Test logging util', function () {
6464

6565
for (const level of ['debug', 'info', 'warn', 'error', 'fatal']) {
6666
it(`Test commandline parameter configuration of log level '${level}'`, async function () {
67+
this.timeout(10000);
6768
// test the specific log level
6869
await testLoglevel(level);
6970
});
@@ -194,4 +195,36 @@ describe('Test logging util', function () {
194195
assert.strictEqual(childLogger.name, 'parent_logger.child_logger');
195196
childLogger.destroy();
196197
});
198+
199+
it('Test logging configure and shutdown', async function () {
200+
await rclnodejs.init();
201+
const logger = rclnodejs.logging.getLogger('config_logger');
202+
203+
// Verify logging works initially
204+
assert.doesNotThrow(() => {
205+
logger.info('Initial message');
206+
});
207+
208+
// Shutdown logging
209+
assert.doesNotThrow(() => {
210+
rclnodejs.logging.shutdown();
211+
});
212+
213+
// Re-configure logging with thread-safe handler
214+
assert.doesNotThrow(() => {
215+
rclnodejs.logging.configure(rclnodejs.Context.defaultContext());
216+
});
217+
218+
// Verify logging works after reconfiguration
219+
assert.doesNotThrow(() => {
220+
logger.info('Message after reconfiguration');
221+
});
222+
223+
// Test invalid context
224+
assert.throws(() => {
225+
rclnodejs.logging.configure({});
226+
}, /context/);
227+
228+
rclnodejs.shutdown();
229+
});
197230
});

test/types/index.test-d.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,10 @@ expectType<boolean>(logger.debug('test msg'));
358358
expectType<boolean>(logger.warn('test msg'));
359359
expectType<boolean>(logger.error('test msg'));
360360
expectType<boolean>(logger.fatal('test msg'));
361+
expectType<void>(
362+
rclnodejs.Logging.configure(rclnodejs.Context.defaultContext())
363+
);
364+
expectType<void>(rclnodejs.Logging.shutdown());
361365
expectType<string>(rclnodejs.Logging.getLoggingDirectory());
362366
expectType<rclnodejs.Logging>(logger.getChild('child'));
363367
expectType<void>(logger.destroy());

types/logging.d.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,18 @@ declare module 'rclnodejs' {
9898
*/
9999
static getLogger(name: string): Logging;
100100

101+
/**
102+
* Configure the logging system with the given context.
103+
*
104+
* @param context - The context to configure logging for.
105+
*/
106+
static configure(context: Context): void;
107+
108+
/**
109+
* Shutdown the logging system.
110+
*/
111+
static shutdown(): void;
112+
101113
/**
102114
* Get the logging directory.
103115
*

0 commit comments

Comments
 (0)