Fix: resolve logic bugs, resource leaks, and error handling issues in JavaScript lib/ (#1410)

Comprehensive review and fix of all JavaScript files under `lib/` that implement
the rclnodejs client library. Eight issues were identified and resolved.

### 1. Incorrect `instanceof` operator precedence (`lib/node.js`)

Changed `(!parameter) instanceof Parameter` to `!(parameter instanceof Parameter)`.
The original expression evaluated `!parameter` to a boolean first, which is never
an instance of `Parameter` — the validation was always `false`, silently accepting
invalid objects as parameter overrides.

### 2. Broken chained comparison in `validType()` (`lib/parameter.js`)

Changed `PARAMETER_NOT_SET <= parameterType <= PARAMETER_STRING_ARRAY` to two
separate comparisons joined with `&&`. JavaScript evaluates chained comparisons
as `(a <= b) <= c`, where `(a <= b)` produces a boolean (0 or 1) — the result
was always `true`, allowing any numeric value to pass type validation.

### 3. Missing `else if` in `_validArray()` (`lib/parameter.js`)

Changed three standalone `if` blocks to `else if` in the array element type
resolution. The conditions are mutually exclusive by design; without `else if`,
later branches could overwrite a correctly resolved `arrayElementType`.

### 4. Typo `_loggger` → `_logger` (`lib/lifecycle_publisher.js`)

Fixed triple-g typo in both the property declaration and usage site. The typo
was consistent so it didn't crash at runtime, but any code referencing the
standard `_logger` spelling would get `undefined`.

### 5. Duplicate getter/setter with `for...in` bug and missing unsubscribe (`lib/time_source.js`)

Removed the first duplicate `get/set isRosTimeActive` definition. It contained
two bugs: `for (const clock in ...)` iterated over array indices (strings), not
clock objects, making the property assignment a no-op; and the `else` branch
incorrectly set `this._node._clockSubscription` instead of
`this._clockSubscription`.

Additionally, the retained setter was missing cleanup logic when ROS time is
deactivated — it subscribed to the clock topic when enabled but never
unsubscribed when disabled. Added an `else` branch to destroy the clock
subscription (matching the pattern in `detachNode()`), preventing an orphaned
subscription from continuing to invoke `_clockCallback` unnecessarily.

### 6. Unhandled promise rejection in service callback (`lib/service.js`)

Added `.catch()` to the `Promise.resolve(this._callback(...)).then(...)` chain.
Without it, if the user's service callback threw or rejected, the rejection was
unhandled — potentially crashing the process or leaving clients hanging.

The error handler uses `Logging.getLogger('rclnodejs').error()` instead of
`debug()` so that service callback errors are visible through the ROS logging
system by default, without requiring the `DEBUG=rclnodejs:service` environment
variable.

### 7. TypeError on unknown distro ID (`lib/distro.js`)

Added null check before accessing `[0]` on the `Array.find()` result. Previously,
calling `getDistroName()` with an ID not in the map would throw
`TypeError: Cannot read properties of undefined`. It now returns `undefined`.

### 8. Feedback callback leak for rejected goals (`lib/action/client.js`)

Added cleanup of the `_feedbackCallbacks` map entry when a goal is rejected in
`processGoalResponse()`. Previously, the callback registered in `sendGoal()` was
never removed for rejected goals, causing unbounded map growth.

### Tests added

- `test/test-parameters.js` — 4 new tests:
  - Rejects non-Parameter objects in parameterOverrides (validates issue 1)
  - Rejects invalid numeric parameter types (validates issue 2)
  - Accepts all known ParameterTypes
  - Validates array parameter types (validates issue 3)
- `test/test-distro.js` — 1 new test:
  - `getDistroName(99999)` returns `undefined` instead of throwing (validates issue 7)

Fix: #1409

Author: minggangw

lib/action/client.js

@@ -126,6 +126,12 @@ class ActionClient extends Entity {
         }
 
         this._goalHandles.set(uuid, goalHandle);
+      } else {
+        // Clean up feedback callback for rejected goals
+        let uuid = ActionUuid.fromMessage(
+          this._sequenceNumberGoalIdMap.get(sequence)
+        ).toString();
+        this._feedbackCallbacks.delete(uuid);
       }
 
       this._pendingGoalRequests.get(sequence).setResult(goalHandle);

lib/distro.js

@@ -65,7 +65,8 @@ const DistroUtils = {
       return process.env.ROS_DISTRO;
     }
 
-    return [...DistroNameIdMap].find(([, val]) => val == distroId)[0];
+    const result = [...DistroNameIdMap].find(([, val]) => val == distroId);
+    return result ? result[0] : undefined;
   },
 
   getKnownDistroNames: function () {

lib/lifecycle_publisher.js

@@ -30,7 +30,7 @@ class LifecyclePublisher extends Publisher {
     super(handle, typeClass, /*topic=*/ '', options);
 
     this._enabled = false;
-    this._loggger = Logging.getLogger('LifecyclePublisher');
+    this._logger = Logging.getLogger('LifecyclePublisher');
   }
 
   /**
@@ -42,7 +42,7 @@ class LifecyclePublisher extends Publisher {
    */
   publish(message) {
     if (!this._enabled) {
-      this._loggger.warn(
+      this._logger.warn(
         `Trying to publish message on the topic ${this.topic}, but the publisher is not activated`
       );
 

lib/node.js

@@ -151,7 +151,7 @@ class Node extends rclnodejs.ShadowNode {
     // override cli parameterOverrides with those specified in options
     if (options.parameterOverrides.length > 0) {
       for (const parameter of options.parameterOverrides) {
-        if ((!parameter) instanceof Parameter) {
+        if (!(parameter instanceof Parameter)) {
           throw new TypeValidationError(
             'parameterOverride',
             parameter,

lib/parameter.js

@@ -864,9 +864,8 @@ function parameterTypeFromValue(value) {
 function validType(parameterType) {
   let result =
     typeof parameterType === 'number' &&
-    ParameterType.PARAMETER_NOT_SET <=
-      parameterType <=
-      ParameterType.PARAMETER_STRING_ARRAY;
+    parameterType >= ParameterType.PARAMETER_NOT_SET &&
+    parameterType <= ParameterType.PARAMETER_STRING_ARRAY;
 
   return result;
 }
@@ -923,14 +922,11 @@ function _validArray(values, type) {
     arrayElementType = ParameterType.PARAMETER_BOOL;
   } else if (type === ParameterType.PARAMETER_BYTE_ARRAY) {
     arrayElementType = PARAMETER_BYTE;
-  }
-  if (type === ParameterType.PARAMETER_INTEGER_ARRAY) {
+  } else if (type === ParameterType.PARAMETER_INTEGER_ARRAY) {
     arrayElementType = ParameterType.PARAMETER_INTEGER;
-  }
-  if (type === ParameterType.PARAMETER_DOUBLE_ARRAY) {
+  } else if (type === ParameterType.PARAMETER_DOUBLE_ARRAY) {
     arrayElementType = ParameterType.PARAMETER_DOUBLE;
-  }
-  if (type === ParameterType.PARAMETER_STRING_ARRAY) {
+  } else if (type === ParameterType.PARAMETER_STRING_ARRAY) {
     arrayElementType = ParameterType.PARAMETER_STRING;
   }
 

lib/service.js

@@ -17,6 +17,7 @@
 const rclnodejs = require('./native_loader.js');
 const DistroUtils = require('./distro.js');
 const Entity = require('./entity.js');
+const Logging = require('./logging.js');
 const debug = require('debug')('rclnodejs:service');
 
 /**
@@ -75,8 +76,8 @@ class Service extends Entity {
 
     const plainObj = request.toPlainObject(this.typedArrayEnabled);
     const response = new Response(this, headerHandle);
-    Promise.resolve(this._callback(plainObj, response)).then(
-      (responseToReturn) => {
+    Promise.resolve(this._callback(plainObj, response))
+      .then((responseToReturn) => {
         if (!response.sent && responseToReturn) {
           responseToReturn = new this._typeClass.Response(responseToReturn);
           const rawResponse = responseToReturn.serialize();
@@ -86,8 +87,11 @@ class Service extends Entity {
         debug(
           `Service has processed the ${this._serviceName} request and sent the response.`
         );
-      }
-    );
+      })
+      .catch((error) => {
+        const logger = Logging.getLogger('rclnodejs');
+        logger.error(`Error processing ${this._serviceName} request: ${error}`);
+      });
   }
 
   static createService(nodeHandle, serviceName, typeClass, options, callback) {

lib/time_source.js

@@ -45,26 +45,6 @@ class TimeSource {
     }
   }
 
-  get isRosTimeActive() {
-    return this._isRosTimeActive;
-  }
-
-  set isRosTimeActive(enabled) {
-    if (this.isRosTimeActive === enabled) return;
-
-    this._isRosTimeActive = enabled;
-    for (const clock in this._associatedClocks) {
-      clock.isRosTimeActive = enabled;
-    }
-
-    if (enabled) {
-      this._subscribeToClockTopic();
-    } else if (this._node && this._clockSubscription) {
-      this._node.destroySubscription(this._clockSubscription);
-      this._node._clockSubscription = null;
-    }
-  }
-
   /**
    * Return status that whether the ROS time is active.
    * @name TimeSource#get:isRosTimeActive
@@ -93,6 +73,9 @@ class TimeSource {
     });
     if (enabled) {
       this._subscribeToClockTopic();
+    } else if (this._node && this._clockSubscription) {
+      this._node.destroySubscription(this._clockSubscription);
+      this._clockSubscription = undefined;
     }
   }
 

test/test-distro.js

@@ -78,4 +78,12 @@ describe('rclnodejs distro utils', function () {
     process.env.ROS_DISTRO = backupEnvar;
     done();
   });
+
+  it('getDistroName with unknown id returns undefined', function () {
+    assert.strictEqual(
+      DistroUtils.getDistroName(99999),
+      undefined,
+      'getDistroName(99999) should return undefined, not throw'
+    );
+  });
 });

test/test-parameters.js

@@ -748,4 +748,84 @@ describe('rclnodejs parameters test suite', function () {
       assert.ok(paramFoundInParamList);
     });
   });
+
+  describe('parameterOverrides validation', function () {
+    const NODE_NAME = 'test_node';
+    let node;
+    this.timeout(60 * 1000);
+
+    afterEach(function () {
+      if (node) node.destroy();
+      rclnodejs.shutdown();
+    });
+
+    it('should reject non-Parameter objects in parameterOverrides', async function () {
+      await rclnodejs.init();
+      const options = new NodeOptions();
+      options.parameterOverrides = [{ name: 'p1', value: 'not a Parameter' }];
+      assert.throws(
+        () =>
+          rclnodejs.createNode(
+            NODE_NAME,
+            '',
+            Context.defaultContext(),
+            options
+          ),
+        (err) => err instanceof rclnodejs.TypeValidationError,
+        'Expected TypeValidationError for non-Parameter parameterOverride'
+      );
+    });
+  });
+
+  describe('parameter type validation', function () {
+    it('validType should reject invalid numeric types', function () {
+      // A Parameter with an out-of-range type should throw on validate()
+      assert.throws(() => {
+        new Parameter('bad', 999, 'value').validate();
+      });
+    });
+
+    it('validType should accept all known ParameterTypes', function () {
+      // NOT_SET type
+      assert.doesNotThrow(() => {
+        new Parameter('p', ParameterType.PARAMETER_NOT_SET).validate();
+      });
+      // BOOL type
+      assert.doesNotThrow(() => {
+        new Parameter('p', ParameterType.PARAMETER_BOOL, true).validate();
+      });
+      // STRING type
+      assert.doesNotThrow(() => {
+        new Parameter('p', ParameterType.PARAMETER_STRING, 'hello').validate();
+      });
+    });
+
+    it('array parameter validation', function () {
+      assert.doesNotThrow(() => {
+        new Parameter('p', ParameterType.PARAMETER_STRING_ARRAY, [
+          'a',
+          'b',
+        ]).validate();
+      });
+      assert.doesNotThrow(() => {
+        new Parameter('p', ParameterType.PARAMETER_INTEGER_ARRAY, [
+          1n,
+          2n,
+        ]).validate();
+      });
+      assert.doesNotThrow(() => {
+        new Parameter(
+          'p',
+          ParameterType.PARAMETER_DOUBLE_ARRAY,
+          [1.0, 2.0]
+        ).validate();
+      });
+      assert.doesNotThrow(() => {
+        new Parameter('p', ParameterType.PARAMETER_BOOL_ARRAY, [
+          true,
+          false,
+        ]).validate();
+      });
+    });
+  });
 });

test/test-time-source.js

@@ -307,10 +307,9 @@ describe('rclnodejs TimeSource testing', function () {
     assert.strictEqual(timeSource.isRosTimeActive, true);
     assert.notStrictEqual(timeSource._clockSubscription, undefined);
 
-    // Disable - currently the implementation does NOT destroy subscription on disable
-    // This test verifies current behavior (even if it might be considered a bug it ensures stability)
+    // Disable - subscription should be destroyed and cleared
     timeSource.isRosTimeActive = false;
     assert.strictEqual(timeSource.isRosTimeActive, false);
-    assert.notStrictEqual(timeSource._clockSubscription, undefined);
+    assert.strictEqual(timeSource._clockSubscription, undefined);
   });
 });