Skip to content

Commit 9122fb3

Browse files
minggangwminggangw
committed
Fix: resolve logic bugs, resource leaks, and error handling issues in JavaScript lib/
1 parent b1f0964 commit 9122fb3

9 files changed

Lines changed: 110 additions & 37 deletions

File tree

‎lib/action/client.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,12 @@ class ActionClient extends Entity {
126126
}
127127

128128
this._goalHandles.set(uuid, goalHandle);
129+
} else {
130+
// Clean up feedback callback for rejected goals
131+
let uuid = ActionUuid.fromMessage(
132+
this._sequenceNumberGoalIdMap.get(sequence)
133+
).toString();
134+
this._feedbackCallbacks.delete(uuid);
129135
}
130136

131137
this._pendingGoalRequests.get(sequence).setResult(goalHandle);

‎lib/distro.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,8 @@ const DistroUtils = {
6565
return process.env.ROS_DISTRO;
6666
}
6767

68-
return [...DistroNameIdMap].find(([, val]) => val == distroId)[0];
68+
const result = [...DistroNameIdMap].find(([, val]) => val == distroId);
69+
return result ? result[0] : undefined;
6970
},
7071

7172
getKnownDistroNames: function () {

‎lib/lifecycle_publisher.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ class LifecyclePublisher extends Publisher {
3030
super(handle, typeClass, /*topic=*/ '', options);
3131

3232
this._enabled = false;
33-
this._loggger = Logging.getLogger('LifecyclePublisher');
33+
this._logger = Logging.getLogger('LifecyclePublisher');
3434
}
3535

3636
/**
@@ -42,7 +42,7 @@ class LifecyclePublisher extends Publisher {
4242
*/
4343
publish(message) {
4444
if (!this._enabled) {
45-
this._loggger.warn(
45+
this._logger.warn(
4646
`Trying to publish message on the topic ${this.topic}, but the publisher is not activated`
4747
);
4848

‎lib/node.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ class Node extends rclnodejs.ShadowNode {
151151
// override cli parameterOverrides with those specified in options
152152
if (options.parameterOverrides.length > 0) {
153153
for (const parameter of options.parameterOverrides) {
154-
if ((!parameter) instanceof Parameter) {
154+
if (!(parameter instanceof Parameter)) {
155155
throw new TypeValidationError(
156156
'parameterOverride',
157157
parameter,

‎lib/parameter.js‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -864,9 +864,8 @@ function parameterTypeFromValue(value) {
864864
function validType(parameterType) {
865865
let result =
866866
typeof parameterType === 'number' &&
867-
ParameterType.PARAMETER_NOT_SET <=
868-
parameterType <=
869-
ParameterType.PARAMETER_STRING_ARRAY;
867+
parameterType >= ParameterType.PARAMETER_NOT_SET &&
868+
parameterType <= ParameterType.PARAMETER_STRING_ARRAY;
870869

871870
return result;
872871
}
@@ -923,14 +922,11 @@ function _validArray(values, type) {
923922
arrayElementType = ParameterType.PARAMETER_BOOL;
924923
} else if (type === ParameterType.PARAMETER_BYTE_ARRAY) {
925924
arrayElementType = PARAMETER_BYTE;
926-
}
927-
if (type === ParameterType.PARAMETER_INTEGER_ARRAY) {
925+
} else if (type === ParameterType.PARAMETER_INTEGER_ARRAY) {
928926
arrayElementType = ParameterType.PARAMETER_INTEGER;
929-
}
930-
if (type === ParameterType.PARAMETER_DOUBLE_ARRAY) {
927+
} else if (type === ParameterType.PARAMETER_DOUBLE_ARRAY) {
931928
arrayElementType = ParameterType.PARAMETER_DOUBLE;
932-
}
933-
if (type === ParameterType.PARAMETER_STRING_ARRAY) {
929+
} else if (type === ParameterType.PARAMETER_STRING_ARRAY) {
934930
arrayElementType = ParameterType.PARAMETER_STRING;
935931
}
936932

‎lib/service.js‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ class Service extends Entity {
7575

7676
const plainObj = request.toPlainObject(this.typedArrayEnabled);
7777
const response = new Response(this, headerHandle);
78-
Promise.resolve(this._callback(plainObj, response)).then(
79-
(responseToReturn) => {
78+
Promise.resolve(this._callback(plainObj, response))
79+
.then((responseToReturn) => {
8080
if (!response.sent && responseToReturn) {
8181
responseToReturn = new this._typeClass.Response(responseToReturn);
8282
const rawResponse = responseToReturn.serialize();
@@ -86,8 +86,10 @@ class Service extends Entity {
8686
debug(
8787
`Service has processed the ${this._serviceName} request and sent the response.`
8888
);
89-
}
90-
);
89+
})
90+
.catch((error) => {
91+
debug(`Error processing ${this._serviceName} request: ${error}`);
92+
});
9193
}
9294

9395
static createService(nodeHandle, serviceName, typeClass, options, callback) {

‎lib/time_source.js‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -45,26 +45,6 @@ class TimeSource {
4545
}
4646
}
4747

48-
get isRosTimeActive() {
49-
return this._isRosTimeActive;
50-
}
51-
52-
set isRosTimeActive(enabled) {
53-
if (this.isRosTimeActive === enabled) return;
54-
55-
this._isRosTimeActive = enabled;
56-
for (const clock in this._associatedClocks) {
57-
clock.isRosTimeActive = enabled;
58-
}
59-
60-
if (enabled) {
61-
this._subscribeToClockTopic();
62-
} else if (this._node && this._clockSubscription) {
63-
this._node.destroySubscription(this._clockSubscription);
64-
this._node._clockSubscription = null;
65-
}
66-
}
67-
6848
/**
6949
* Return status that whether the ROS time is active.
7050
* @name TimeSource#get:isRosTimeActive

‎test/test-distro.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,4 +78,12 @@ describe('rclnodejs distro utils', function () {
7878
process.env.ROS_DISTRO = backupEnvar;
7979
done();
8080
});
81+
82+
it('getDistroName with unknown id returns undefined', function () {
83+
assert.strictEqual(
84+
DistroUtils.getDistroName(99999),
85+
undefined,
86+
'getDistroName(99999) should return undefined, not throw'
87+
);
88+
});
8189
});

‎test/test-parameters.js‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -748,4 +748,84 @@ describe('rclnodejs parameters test suite', function () {
748748
assert.ok(paramFoundInParamList);
749749
});
750750
});
751+
752+
describe('parameterOverrides validation', function () {
753+
const NODE_NAME = 'test_node';
754+
let node;
755+
this.timeout(60 * 1000);
756+
757+
afterEach(function () {
758+
if (node) node.destroy();
759+
rclnodejs.shutdown();
760+
});
761+
762+
it('should reject non-Parameter objects in parameterOverrides', async function () {
763+
await rclnodejs.init();
764+
const options = new NodeOptions();
765+
options.parameterOverrides = [{ name: 'p1', value: 'not a Parameter' }];
766+
assert.throws(
767+
() =>
768+
rclnodejs.createNode(
769+
NODE_NAME,
770+
'',
771+
Context.defaultContext(),
772+
options
773+
),
774+
(err) => err instanceof rclnodejs.TypeValidationError,
775+
'Expected TypeValidationError for non-Parameter parameterOverride'
776+
);
777+
});
778+
});
779+
780+
describe('parameter type validation', function () {
781+
it('validType should reject invalid numeric types', function () {
782+
// A Parameter with an out-of-range type should throw on validate()
783+
assert.throws(() => {
784+
new Parameter('bad', 999, 'value').validate();
785+
});
786+
});
787+
788+
it('validType should accept all known ParameterTypes', function () {
789+
// NOT_SET type
790+
assert.doesNotThrow(() => {
791+
new Parameter('p', ParameterType.PARAMETER_NOT_SET).validate();
792+
});
793+
// BOOL type
794+
assert.doesNotThrow(() => {
795+
new Parameter('p', ParameterType.PARAMETER_BOOL, true).validate();
796+
});
797+
// STRING type
798+
assert.doesNotThrow(() => {
799+
new Parameter('p', ParameterType.PARAMETER_STRING, 'hello').validate();
800+
});
801+
});
802+
803+
it('array parameter validation', function () {
804+
assert.doesNotThrow(() => {
805+
new Parameter('p', ParameterType.PARAMETER_STRING_ARRAY, [
806+
'a',
807+
'b',
808+
]).validate();
809+
});
810+
assert.doesNotThrow(() => {
811+
new Parameter('p', ParameterType.PARAMETER_INTEGER_ARRAY, [
812+
1n,
813+
2n,
814+
]).validate();
815+
});
816+
assert.doesNotThrow(() => {
817+
new Parameter(
818+
'p',
819+
ParameterType.PARAMETER_DOUBLE_ARRAY,
820+
[1.0, 2.0]
821+
).validate();
822+
});
823+
assert.doesNotThrow(() => {
824+
new Parameter('p', ParameterType.PARAMETER_BOOL_ARRAY, [
825+
true,
826+
false,
827+
]).validate();
828+
});
829+
});
830+
});
751831
});

0 commit comments

Comments
 (0)