Address comments

Author: minggangw

lib/action/client.js

@@ -95,7 +95,8 @@ class ActionClient extends Entity {
 
     // Enable feedback subscription content filter optimization.
     // Only supported on ROS2 Rolling and only effective when the native
-    // binding provides the required functions.
+    // binding provides the required functions AND the RMW implementation
+    // actually supports content filtering on the feedback subscription.
     this._enableFeedbackMsgOptimization =
       this._options.enableFeedbackMsgOptimization === true &&
       DistroUtils.getDistroId() >= DistroUtils.DistroId.ROLLING &&
@@ -115,6 +116,29 @@ class ActionClient extends Entity {
       this.qos.statusSubQosProfile
     );
 
+    // Probe that the RMW actually supports content filtering before enabling
+    // the optimization.  Content filter support is an RMW-level capability,
+    // so testing any subscription is sufficient.  If unsupported, silently
+    // disable the optimization to avoid a fatal crash in the DDS layer.
+    if (this._enableFeedbackMsgOptimization) {
+      try {
+        const probeSub = node.createSubscription(
+          this._typeClass.impl.FeedbackMessage,
+          actionName + '/_action/feedback',
+          () => {}
+        );
+        const supported =
+          typeof probeSub.isContentFilterSupported === 'function' &&
+          probeSub.isContentFilterSupported();
+        probeSub.destroy();
+        if (!supported) {
+          this._enableFeedbackMsgOptimization = false;
+        }
+      } catch {
+        this._enableFeedbackMsgOptimization = false;
+      }
+    }
+
     node._addActionClient(this);
   }
 

test/test-action-client.js

@@ -336,7 +336,7 @@ describe('rclnodejs action client', function () {
       client.destroy();
     });
 
-    it('Test send goal with optimization enabled', async function () {
+    it('Test does not affect normal feedback reception', async function () {
       let client = new rclnodejs.ActionClient(node, fibonacci, 'fibonacci', {
         enableFeedbackMsgOptimization: true,
       });
@@ -360,11 +360,73 @@ describe('rclnodejs action client', function () {
 
       await goalHandle.getResult();
       assert.ok(goalHandle.isSucceeded());
+      assert.ok(feedbackCallback.calledOnce);
 
       client.destroy();
     });
 
-    it('Test send multiple goals with optimization enabled', async function () {
+    it('Test handles multiple goals with selective feedback', async function () {
+      let client = new rclnodejs.ActionClient(node, fibonacci, 'fibonacci', {
+        enableFeedbackMsgOptimization: true,
+      });
+
+      let goal1Uuid = ActionUuid.randomMessage();
+      let goal2Uuid = ActionUuid.randomMessage();
+
+      let feedback1Callback = sinon.spy();
+      let feedback2Callback = sinon.spy();
+
+      // Only publish feedback for the first goal
+      publishFeedback = goal1Uuid;
+
+      let result = await client.waitForServer(2000);
+      assert.ok(result);
+
+      const [goal1Handle, goal2Handle] = await Promise.all([
+        client.sendGoal(new Fibonacci.Goal(), feedback1Callback, goal1Uuid),
+        client.sendGoal(new Fibonacci.Goal(), feedback2Callback, goal2Uuid),
+      ]);
+
+      await goal1Handle.getResult();
+      await goal2Handle.getResult();
+
+      // Only first goal should have received feedback
+      assert.ok(feedback1Callback.calledOnce);
+      assert.ok(feedback2Callback.notCalled);
+
+      client.destroy();
+    });
+
+    it('Test cancel goal then send new goal', async function () {
+      let client = new rclnodejs.ActionClient(node, fibonacci, 'fibonacci', {
+        enableFeedbackMsgOptimization: true,
+      });
+
+      let result = await client.waitForServer(2000);
+      assert.ok(result);
+
+      let goalHandle = await client.sendGoal(new Fibonacci.Goal());
+      assert.ok(goalHandle.isAccepted());
+
+      result = await goalHandle.cancelGoal();
+      assert.ok(result);
+
+      assert.strictEqual(
+        ActionUuid.fromMessage(result.goals_canceling[0].goal_id).toString(),
+        ActionUuid.fromMessage(goalHandle.goalId).toString()
+      );
+
+      // Send another goal after cancel - should still work
+      let goalHandle2 = await client.sendGoal(new Fibonacci.Goal());
+      assert.ok(goalHandle2.isAccepted());
+
+      let result2 = await goalHandle2.getResult();
+      assert.ok(result2);
+
+      client.destroy();
+    });
+
+    it('Test send multiple goals (3)', async function () {
       let client = new rclnodejs.ActionClient(node, fibonacci, 'fibonacci', {
         enableFeedbackMsgOptimization: true,
       });
@@ -395,24 +457,35 @@ describe('rclnodejs action client', function () {
       client.destroy();
     });
 
-    it('Test cancel goal with optimization enabled', async function () {
+    it('Test handles more than 6 goals gracefully', async function () {
+      // The DDS content filter limit is 6 concurrent goals (100 params / 16 per goal).
+      // When exceeded, optimization auto-disables but goals should still work.
       let client = new rclnodejs.ActionClient(node, fibonacci, 'fibonacci', {
         enableFeedbackMsgOptimization: true,
       });
 
+      let feedbackCallback = sinon.spy();
+      let goalUuid = ActionUuid.randomMessage();
+      publishFeedback = goalUuid;
+
       let result = await client.waitForServer(2000);
       assert.ok(result);
 
-      let goalHandle = await client.sendGoal(new Fibonacci.Goal());
-      assert.ok(goalHandle.isAccepted());
-
-      result = await goalHandle.cancelGoal();
-      assert.ok(result);
+      // Send 7 goals sequentially - exceeds the 6 goal content filter limit
+      let handles = [];
+      for (let i = 0; i < 7; i++) {
+        let uuid = i === 0 ? goalUuid : undefined;
+        let cb = i === 0 ? feedbackCallback : undefined;
+        let h = await client.sendGoal(new Fibonacci.Goal(), cb, uuid);
+        assert.ok(h.isAccepted());
+        handles.push(h);
+      }
 
-      assert.strictEqual(
-        ActionUuid.fromMessage(result.goals_canceling[0].goal_id).toString(),
-        ActionUuid.fromMessage(goalHandle.goalId).toString()
-      );
+      // Wait for all results
+      for (const h of handles) {
+        let r = await h.getResult();
+        assert.ok(r);
+      }
 
       client.destroy();
     });

test/test-subscription-content-filter.js

@@ -480,36 +480,3 @@ describe('subscription content-filtering', function () {
     done();
   });
 });
-
-describe('subscription isContentFilterSupported', function () {
-  this.timeout(30 * 1000);
-
-  beforeEach(async function () {
-    await rclnodejs.init();
-    this.node = new Node('cft_support_test_node');
-  });
-
-  afterEach(function () {
-    this.node.destroy();
-    rclnodejs.shutdown();
-  });
-
-  it('isContentFilterSupported returns boolean matching RMW capability', function (done) {
-    const typeclass = 'std_msgs/msg/Int16';
-    const subscription = this.node.createSubscription(
-      typeclass,
-      TOPIC,
-      (msg) => {}
-    );
-
-    const supported = subscription.isContentFilterSupported();
-    assert.strictEqual(typeof supported, 'boolean');
-
-    // isContentFilterSupported requires rolling; on older distros it returns false
-    const isRolling = DistroUtils.getDistroId() >= DistroUtils.DistroId.ROLLING;
-    const expectedSupported = isRolling && isContentFilteringSupported();
-    assert.strictEqual(supported, expectedSupported);
-
-    done();
-  });
-});