Skip to content

Commit e01fbf7

Browse files
minggangwminggangw
authored
Fix subscription content-filtering flakiness (#1369)
This pull request fixes subscription content-filtering flakiness by addressing memory allocation bugs and improving error handling in the content filter operations. **Changes:** - Fixed memory allocation size calculation from `sizeof(char*)` to `sizeof(char)` in CreateSubscription and SetContentFilter functions - Replaced macro-based error handling with explicit error checking and cleanup in SetContentFilter and ClearContentFilter - Added proper resource finalization calls for content filter options Fix: #1368
1 parent b823a2d commit e01fbf7

File tree

3 files changed

+143
-80
lines changed

3 files changed

+143
-80
lines changed

‎src/rcl_subscription_bindings.cpp‎

Lines changed: 95 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@ Napi::Value RclTake(const Napi::CallbackInfo& info) {
3838
rcl_ret_t ret = rcl_take(subscription, msg_taken, nullptr, nullptr);
3939

4040
if (ret != RCL_RET_OK && ret != RCL_RET_SUBSCRIPTION_TAKE_FAILED) {
41+
std::string error_string = rcl_get_error_string().str;
4142
rcl_reset_error();
42-
Napi::Error::New(env, rcl_get_error_string().str)
43-
.ThrowAsJavaScriptException();
43+
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
4444
return Napi::Boolean::New(env, false);
4545
}
4646

@@ -99,7 +99,7 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
9999
for (int i = 0; i < argc; i++) {
100100
std::string arg = jsArgv.Get(i).As<Napi::String>().Utf8Value();
101101
int len = arg.length() + 1;
102-
argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char*)));
102+
argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char)));
103103
snprintf(argv[i], len, "%s", arg.c_str());
104104
}
105105
}
@@ -109,9 +109,9 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
109109
expression.c_str(), argc, (const char**)argv, &subscription_ops);
110110

111111
if (ret != RCL_RET_OK) {
112+
std::string error_string = rcl_get_error_string().str;
112113
rcl_reset_error();
113-
Napi::Error::New(env, rcl_get_error_string().str)
114-
.ThrowAsJavaScriptException();
114+
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
115115
}
116116

117117
if (argc) {
@@ -120,33 +120,46 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
120120
}
121121
free(argv);
122122
}
123+
124+
if (ret != RCL_RET_OK) {
125+
free(subscription);
126+
return env.Undefined();
127+
}
123128
}
124129
}
125130

126131
const rosidl_message_type_support_t* ts =
127132
GetMessageTypeSupport(package_name, message_sub_folder, message_name);
128133

129134
if (ts) {
130-
THROW_ERROR_IF_NOT_EQUAL(
131-
RCL_RET_OK,
132-
rcl_subscription_init(subscription, node, ts, topic.c_str(),
133-
&subscription_ops),
134-
rcl_get_error_string().str);
135+
rcl_ret_t ret = rcl_subscription_init(subscription, node, ts, topic.c_str(),
136+
&subscription_ops);
137+
if (ret != RCL_RET_OK) {
138+
std::string error_msg = rcl_get_error_string().str;
139+
rcl_reset_error();
140+
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
141+
free(subscription);
142+
return env.Undefined();
143+
}
135144

136145
auto js_obj = RclHandle::NewInstance(
137146
env, subscription, node_handle, [node, env](void* ptr) {
138147
rcl_subscription_t* subscription =
139148
reinterpret_cast<rcl_subscription_t*>(ptr);
140149
rcl_ret_t ret = rcl_subscription_fini(subscription, node);
141150
free(ptr);
142-
THROW_ERROR_IF_NOT_EQUAL_NO_RETURN(RCL_RET_OK, ret,
143-
rcl_get_error_string().str);
151+
if (ret != RCL_RET_OK) {
152+
std::string error_msg = rcl_get_error_string().str;
153+
rcl_reset_error();
154+
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
155+
}
144156
});
145157

146158
return js_obj;
147159
} else {
148160
Napi::Error::New(env, GetErrorMessageAndClear())
149161
.ThrowAsJavaScriptException();
162+
free(subscription);
150163
return env.Undefined();
151164
}
152165
}
@@ -235,7 +248,7 @@ Napi::Value SetContentFilter(const Napi::CallbackInfo& info) {
235248
for (int i = 0; i < argc; i++) {
236249
std::string arg = jsArgv.Get(i).As<Napi::String>().Utf8Value();
237250
int len = arg.length() + 1;
238-
argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char*)));
251+
argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char)));
239252
snprintf(argv[i], len, "%s", arg.c_str());
240253
}
241254
}
@@ -245,15 +258,23 @@ Napi::Value SetContentFilter(const Napi::CallbackInfo& info) {
245258
rcl_subscription_content_filter_options_t options =
246259
rcl_get_zero_initialized_subscription_content_filter_options();
247260

248-
THROW_ERROR_IF_NOT_EQUAL(
249-
RCL_RET_OK,
250-
rcl_subscription_content_filter_options_set(
251-
subscription, expression.c_str(), argc, (const char**)argv, &options),
252-
rcl_get_error_string().str);
261+
rcl_ret_t ret = rcl_subscription_content_filter_options_set(
262+
subscription, expression.c_str(), argc, (const char**)argv, &options);
253263

254-
THROW_ERROR_IF_NOT_EQUAL(
255-
RCL_RET_OK, rcl_subscription_set_content_filter(subscription, &options),
256-
rcl_get_error_string().str);
264+
if (ret != RCL_RET_OK) {
265+
if (argc) {
266+
for (int i = 0; i < argc; i++) {
267+
free(argv[i]);
268+
}
269+
free(argv);
270+
}
271+
std::string error_string = rcl_get_error_string().str;
272+
rcl_reset_error();
273+
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
274+
return env.Undefined();
275+
}
276+
277+
ret = rcl_subscription_set_content_filter(subscription, &options);
257278

258279
if (argc) {
259280
for (int i = 0; i < argc; i++) {
@@ -262,6 +283,23 @@ Napi::Value SetContentFilter(const Napi::CallbackInfo& info) {
262283
free(argv);
263284
}
264285

286+
rcl_ret_t fini_ret =
287+
rcl_subscription_content_filter_options_fini(subscription, &options);
288+
289+
if (ret != RCL_RET_OK) {
290+
std::string error_string = rcl_get_error_string().str;
291+
rcl_reset_error();
292+
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
293+
return env.Undefined();
294+
}
295+
296+
if (fini_ret != RCL_RET_OK) {
297+
std::string error_string = rcl_get_error_string().str;
298+
rcl_reset_error();
299+
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
300+
return env.Undefined();
301+
}
302+
265303
return Napi::Boolean::New(env, true);
266304
}
267305

@@ -277,15 +315,33 @@ Napi::Value ClearContentFilter(const Napi::CallbackInfo& info) {
277315
rcl_subscription_content_filter_options_t options =
278316
rcl_get_zero_initialized_subscription_content_filter_options();
279317

280-
THROW_ERROR_IF_NOT_EQUAL(
281-
RCL_RET_OK,
282-
rcl_subscription_content_filter_options_init(
283-
subscription, "", 0, (const char**)nullptr, &options),
284-
rcl_get_error_string().str);
318+
rcl_ret_t ret = rcl_subscription_content_filter_options_init(
319+
subscription, "", 0, (const char**)nullptr, &options);
320+
321+
if (ret != RCL_RET_OK) {
322+
std::string error_string = rcl_get_error_string().str;
323+
rcl_reset_error();
324+
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
325+
return env.Undefined();
326+
}
327+
328+
ret = rcl_subscription_set_content_filter(subscription, &options);
329+
rcl_ret_t fini_ret =
330+
rcl_subscription_content_filter_options_fini(subscription, &options);
331+
332+
if (ret != RCL_RET_OK) {
333+
std::string error_string = rcl_get_error_string().str;
334+
rcl_reset_error();
335+
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
336+
return env.Undefined();
337+
}
285338

286-
THROW_ERROR_IF_NOT_EQUAL(
287-
RCL_RET_OK, rcl_subscription_set_content_filter(subscription, &options),
288-
rcl_get_error_string().str);
339+
if (fini_ret != RCL_RET_OK) {
340+
std::string error_string = rcl_get_error_string().str;
341+
rcl_reset_error();
342+
Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
343+
return env.Undefined();
344+
}
289345

290346
return Napi::Boolean::New(env, true);
291347
}
@@ -303,9 +359,9 @@ Napi::Value GetContentFilter(const Napi::CallbackInfo& info) {
303359

304360
rcl_ret_t ret = rcl_subscription_get_content_filter(subscription, &options);
305361
if (ret != RCL_RET_OK) {
306-
Napi::Error::New(env, rcl_get_error_string().str)
307-
.ThrowAsJavaScriptException();
362+
std::string error_msg = rcl_get_error_string().str;
308363
rcl_reset_error();
364+
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
309365
return env.Undefined();
310366
}
311367

@@ -331,9 +387,9 @@ Napi::Value GetContentFilter(const Napi::CallbackInfo& info) {
331387
rcl_ret_t fini_ret =
332388
rcl_subscription_content_filter_options_fini(subscription, &options);
333389
if (fini_ret != RCL_RET_OK) {
334-
Napi::Error::New(env, rcl_get_error_string().str)
335-
.ThrowAsJavaScriptException();
390+
std::string error_msg = rcl_get_error_string().str;
336391
rcl_reset_error();
392+
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
337393
return env.Undefined();
338394
}
339395

@@ -347,9 +403,12 @@ Napi::Value GetPublisherCount(const Napi::CallbackInfo& info) {
347403
RclHandle::Unwrap(info[0].As<Napi::Object>())->ptr());
348404

349405
size_t count = 0;
350-
THROW_ERROR_IF_NOT_EQUAL(
351-
rcl_subscription_get_publisher_count(subscription, &count), RCL_RET_OK,
352-
rcl_get_error_string().str);
406+
rcl_ret_t ret = rcl_subscription_get_publisher_count(subscription, &count);
407+
if (ret != RCL_RET_OK) {
408+
std::string error_msg = rcl_get_error_string().str;
409+
rcl_reset_error();
410+
Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
411+
}
353412

354413
return Napi::Number::New(env, count);
355414
}

‎test/test-clock-event.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ describe('ClockEvent', function () {
4242
await event.waitUntilSteady(clock, until.nanoseconds);
4343
const end = Date.now();
4444

45-
assert(end - start >= 1000);
45+
assert(end - start >= 950);
4646
});
4747

4848
it('should wait until system time', async function () {
@@ -55,7 +55,7 @@ describe('ClockEvent', function () {
5555
await event.waitUntilSystem(clock, until.nanoseconds);
5656
const end = Date.now();
5757

58-
assert(end - start >= 1000);
58+
assert(end - start >= 950);
5959
});
6060

6161
it('should wait until ros time', async function () {
@@ -72,7 +72,7 @@ describe('ClockEvent', function () {
7272
await event.waitUntilRos(clock, until.nanoseconds);
7373
const end = Date.now();
7474

75-
assert(end - start >= 1000);
75+
assert(end - start >= 950);
7676
});
7777

7878
it('should set and clear event', function () {

‎test/test-type-description-service.js‎

Lines changed: 45 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -85,51 +85,55 @@ describe('type description service test suite', function () {
8585
});
8686

8787
it('Test type description service configured by parameter', function (done) {
88-
exec(
89-
'ros2 param list /test_type_description_service',
90-
(error, stdout, stderr) => {
91-
if (error || stderr) {
92-
done(
93-
new Error(
94-
`Test type description service configured by parameter failed. Error: ${error}, Stderr: ${stderr}`
95-
)
96-
);
97-
return;
98-
}
99-
if (stdout.includes('start_type_description_service')) {
100-
done();
101-
} else {
102-
done(
103-
new Error("'start_type_description_service' not found in stdout.")
104-
);
88+
setTimeout(() => {
89+
exec(
90+
'ros2 param list /test_type_description_service',
91+
(error, stdout, stderr) => {
92+
if (error || stderr) {
93+
done(
94+
new Error(
95+
`Test type description service configured by parameter failed. Error: ${error}, Stderr: ${stderr}`
96+
)
97+
);
98+
return;
99+
}
100+
if (stdout.includes('start_type_description_service')) {
101+
done();
102+
} else {
103+
done(
104+
new Error("'start_type_description_service' not found in stdout.")
105+
);
106+
}
105107
}
106-
}
107-
);
108+
);
109+
}, 1000);
108110
});
109111

110112
it('Test start_type_description_service parameter value', function (done) {
111-
exec(
112-
'ros2 param get /test_type_description_service start_type_description_service',
113-
(error, stdout, stderr) => {
114-
if (error || stderr) {
115-
done(
116-
new Error(
117-
`Test type description service configured by parameter failed. Error: ${error}, Stderr: ${stderr}`
118-
)
119-
);
120-
return;
121-
}
122-
if (stdout.includes('Boolean value is: True')) {
123-
done();
124-
} else {
125-
console.log(stdout);
126-
done(
127-
new Error(
128-
"'start_type_description_service param value' not found in stdout."
129-
)
130-
);
113+
setTimeout(() => {
114+
exec(
115+
'ros2 param get /test_type_description_service start_type_description_service',
116+
(error, stdout, stderr) => {
117+
if (error || stderr) {
118+
done(
119+
new Error(
120+
`Test type description service configured by parameter failed. Error: ${error}, Stderr: ${stderr}`
121+
)
122+
);
123+
return;
124+
}
125+
if (stdout.includes('Boolean value is: True')) {
126+
done();
127+
} else {
128+
console.log(stdout);
129+
done(
130+
new Error(
131+
"'start_type_description_service param value' not found in stdout."
132+
)
133+
);
134+
}
131135
}
132-
}
133-
);
136+
);
137+
}, 1000);
134138
});
135139
});

0 commit comments

Comments
 (0)