Support feedback subscription content filter for action client by minggangw · Pull Request #1457 · RobotWebTools/rclnodejs

minggangw · 2026-03-30T04:42:57Z

Add enableFeedbackMsgOptimization option to ActionClient that uses the DDS content filter to avoid receiving irrelevant feedback messages when multiple action clients share the same action server. This mirrors the rclpy implementation (ros2/rclpy#1633).

When enabled, goal IDs are added to the feedback subscription's content filter on goal acceptance and removed on terminal status, result receipt, or cancel. The optimization supports up to 6 concurrent goals (limited by the DDS 100-parameter maximum) and auto-disables gracefully when the limit is exceeded or the RMW does not support content filtering.

Changes:

lib/action/client.js: Add content filter add/remove calls matching rclpy's pattern — direct native API calls with try/catch and warning on error, no JS-side tracking structures.
src/rcl_action_client_bindings.cpp: N-API bindings for rcl_action_client_configure_feedback_subscription_filter_add/remove_goal_id, guarded by ROS_VERSION >= 5000 (Rolling).
src/executor.cpp, src/executor.h: Add condition variable synchronization so the background thread waits after uv_async_send until the main thread finishes ExecuteReadyHandles. This prevents a data race where the background thread re-enters rcl_wait (holding subscription references) while the main thread modifies the content filter. Use RAII guard to ensure notification on exception.
types/action_client.d.ts: Add enableFeedbackMsgOptimization option.
test/test-action-client.js: 7 new tests covering default/enable flag, normal feedback, multiple goals with optimization, cancel+new goal, concurrent goals, and >6 goals overflow with auto-disable.

Fix: #1456

Copilot

Pull request overview

Adds an optional “feedback message optimization” path for action clients by configuring a feedback subscription content filter with active goal IDs (Rolling-only / native-binding dependent), plus corresponding typings and tests.

Changes:

Add enableFeedbackMsgOptimization option to ActionClient (JS + TS typings) to use a feedback subscription content filter keyed on goal IDs.
Expose new native bindings to add/remove goal IDs from the feedback subscription content filter (ROS 2 Rolling only).
Add/extend tests covering the new option and (attempted) content-filter support detection.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
types/action_client.d.ts	Adds the `enableFeedbackMsgOptimization` option to the ActionClient constructor typings/docs.
lib/action/client.js	Implements enabling/disabling optimization and add/remove goal ID calls around goal lifecycle.
src/rcl_action_client_bindings.cpp	Adds Rolling-only N-API exports to add/remove goal IDs in the feedback subscription filter.
test/test-action-client.js	Adds tests validating option defaulting/gating and basic goal flows with the option enabled.
test/test-subscription-content-filter.js	Adds a test suite intended to validate content-filter support detection via a new API.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-30T04:46:41Z

test/test-subscription-content-filter.js

+  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);


subscription.isContentFilterSupported() is called here, but the Subscription class in lib/subscription.js does not define this method and there is no other implementation found in the repo. This will throw at runtime and fail the test. Either implement isContentFilterSupported() on Subscription (and any needed native binding) or update the test to use an existing API to determine content-filter support.

Copilot · 2026-03-30T04:46:41Z

lib/action/client.js

+    this._enableFeedbackMsgOptimization =
+      this._options.enableFeedbackMsgOptimization === true &&
+      DistroUtils.getDistroId() >= DistroUtils.DistroId.ROLLING &&
+      typeof rclnodejs.actionConfigureFeedbackSubFilterAddGoalId === 'function';


_enableFeedbackMsgOptimization is enabled based on the presence of actionConfigureFeedbackSubFilterAddGoalId, but later code also calls actionConfigureFeedbackSubFilterRemoveGoalId. For safety, gate on both functions existing (or gate per-call) to avoid a runtime exception if a build/export ever provides only one of the two functions.

Suggested change

typeof rclnodejs.actionConfigureFeedbackSubFilterAddGoalId === 'function';

typeof rclnodejs.actionConfigureFeedbackSubFilterAddGoalId === 'function' &&

typeof rclnodejs.actionConfigureFeedbackSubFilterRemoveGoalId === 'function';

Copilot · 2026-03-30T04:46:41Z

lib/action/client.js

-    );
+    deferred.setDoneCallback(() => {
+      this._removePendingResultRequest(sequenceNumber);
+      this._feedbackSubFilterRemoveGoalId(goalHandle.goalId);


Goal IDs are removed from the feedback subscription filter in multiple places (here in _getResult completion, also in processStatusMessage terminal-state handling, and in _cancelGoal). If the native remove operation is not idempotent, a second removal can throw and will disable optimization unnecessarily. Consider tracking which goal IDs have been added/removed (e.g., a Set) and only removing once, or centralize removal in a single lifecycle point.

Suggested change

this._feedbackSubFilterRemoveGoalId(goalHandle.goalId);

// Ensure feedback subscription filter removal is idempotent for this goal ID.

if (!this._removedFeedbackGoalIds) {

this._removedFeedbackGoalIds = new Set();

}

if (!this._removedFeedbackGoalIds.has(goalHandle.goalId)) {

this._feedbackSubFilterRemoveGoalId(goalHandle.goalId);

this._removedFeedbackGoalIds.add(goalHandle.goalId);

}

lib/action/client.js

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-30T05:30:34Z

lib/action/client.js

+    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();


The probe subscription is only destroyed on the success path. If isContentFilterSupported() throws (or any error occurs after createSubscription succeeds), probeSub.destroy() is skipped and the probe subscription may leak. Use a finally block (or a nested try/finally) to always destroy the probe subscription when it is created.

Copilot · 2026-03-30T05:30:34Z

lib/action/client.js

+    this._feedbackSubFilterRemoveGoalId(goalHandle.goalId);
+


_cancelGoal() removes the goal ID from the feedback filter immediately after sending the cancel request. If the cancel request is rejected (or takes time to be accepted), this can prematurely stop feedback for an active goal. Consider removing the goal ID only after the goal transitions to a terminal state (e.g., in processStatusMessage), or after a cancel response confirms the goal is canceling/canceled.

Suggested change

this._feedbackSubFilterRemoveGoalId(goalHandle.goalId);

Copilot · 2026-03-30T05:30:35Z

test/test-action-client.js

+      // If native API is available, it should be enabled; otherwise disabled
+      if (isFeedbackFilterSupported()) {
+        assert.strictEqual(client._enableFeedbackMsgOptimization, true);
+      } else {
+        assert.strictEqual(client._enableFeedbackMsgOptimization, false);
+      }


This test assumes the option will be enabled whenever the native export exists, but ActionClient also disables optimization when the RMW doesn't support content filtering (via the probe subscription). On Rolling with an RMW that lacks CFT support, this assertion will fail. Compute the expected value using the same gating as the implementation (e.g., check subscription.isContentFilterSupported() / RMW capability) or assert only that enabling never throws and that behavior is unchanged when disabled.

Suggested change

// If native API is available, it should be enabled; otherwise disabled

if (isFeedbackFilterSupported()) {

assert.strictEqual(client._enableFeedbackMsgOptimization, true);

} else {

assert.strictEqual(client._enableFeedbackMsgOptimization, false);

}

// Ensure the option is recognized and results in a boolean flag;

// the actual enabled state may depend on RMW/content filter support.

assert.strictEqual(

typeof client._enableFeedbackMsgOptimization,

'boolean'

);

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-30T06:08:11Z

lib/action/client.js

+            this._feedbackSubFilterRemoveGoalId(
+              statusMessage.goal_info.goal_id
+            );


Goal IDs are removed from the feedback filter here when a terminal status is observed, but they are also removed in _getResult()'s done callback (and currently in _cancelGoal()). This creates multiple removal paths for the same goal ID, which can trigger errors from the native API and disable optimization unexpectedly. Consolidate removal to a single lifecycle point, or track added IDs and make removal idempotent (only remove if it was added and not already removed).

Suggested change

this._feedbackSubFilterRemoveGoalId(

statusMessage.goal_info.goal_id

);

Copilot · 2026-03-30T06:08:12Z

lib/action/client.js

+  _feedbackSubFilterAddGoalId(goalId) {
+    if (!this._enableFeedbackMsgOptimization) return;
+    try {
+      rclnodejs.actionConfigureFeedbackSubFilterAddGoalId(
+        this.handle,
+        Buffer.from(goalId.uuid)
+      );
+    } catch (e) {
+      this._enableFeedbackMsgOptimization = false;
+      this._node
+        .getLogger()
+        .warn(
+          `${e.message}\nFeedback message optimization is automatically disabled.`
+        );
+    }


When an add/remove call throws, the code disables _enableFeedbackMsgOptimization, but it does not attempt to clear/undo any goal IDs that were already added to the underlying DDS content filter. After this point, new goals will no longer be added to the filter, so feedback for future goals may be filtered out indefinitely. If you auto-disable, also revert the subscription back to an unfiltered state (e.g., best-effort remove all previously-added goal IDs / clear the filter) and track which IDs were successfully added so cleanup is possible.

Copilot · 2026-03-30T06:08:12Z

test/test-subscription-content-filter.js

    assert.ok(!subscription || !subscription.hasContentFilter());

    done();
  });


This file previously had coverage for Subscription.isContentFilterSupported(), including the Rolling guard and RMW capability expectation. With that block removed, there doesn't appear to be a test that directly asserts this API’s contract anymore (even though ActionClient now relies on it for probing). Consider restoring that test here or relocating it so Subscription.isContentFilterSupported() remains covered independently of action-client behavior.

Suggested change

});

});

it('Subscription.isContentFilterSupported matches expected capabilities', function () {

const Subscription = rclnodejs.Subscription;

const supported = Subscription.isContentFilterSupported();

const expected = isContentFilteringSupported();

assert.strictEqual(typeof supported, 'boolean');

assert.strictEqual(supported, expected);

});

coveralls · 2026-03-30T06:15:45Z

coverage: 85.861% (-0.1%) from 85.992%
when pulling d3e4283 on minggangw:fix-1456
into dfe5e64 on RobotWebTools:develop.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T03:25:26Z

src/executor.cpp

 void Executor::DoWork(uv_async_t* handle) {
  Executor* executor = reinterpret_cast<Executor*>(handle->data);
  executor->ExecuteReadyHandles();
+
+  // Signal the background thread that it is safe to re-enter rcl_wait.
+  {
+    std::lock_guard<std::mutex> lock(executor->work_done_mutex_);
+    executor->work_pending_ = false;
+  }
+  executor->work_done_cv_.notify_one();


In DoWork(), work_pending_ is cleared and the condvar is notified only after ExecuteReadyHandles() returns. If ExecuteReadyHandles() (or downstream N-API calls) throws a C++ exception, the background thread can block forever waiting on work_done_cv_. Consider wrapping ExecuteReadyHandles() in a try/finally-style guard (e.g., RAII) to always clear work_pending_ and notify, even on exceptions.

lib/action/client.js

test/test-action-client.js

Add enableFeedbackMsgOptimization option to ActionClient that uses the DDS content filter to avoid receiving irrelevant feedback messages when multiple action clients share the same action server. This mirrors the rclpy implementation (ros2/rclpy#1633). When enabled, goal IDs are added to the feedback subscription's content filter on goal acceptance and removed on terminal status, result receipt, or cancel. The optimization supports up to 6 concurrent goals (limited by the DDS 100-parameter maximum) and auto-disables gracefully when the limit is exceeded or the RMW does not support content filtering. Changes: - lib/action/client.js: Add content filter lifecycle management with idempotent removal tracking (_filterGoalIds, _filterGoalIdBuffers) and graceful disable-and-cleanup on error. - src/rcl_action_client_bindings.cpp: N-API bindings for rcl_action_client_configure_feedback_subscription_filter_add/remove_goal_id, guarded by ROS_VERSION >= 5000 (Rolling). - src/executor.cpp, src/executor.h: Add condition variable synchronization so the background thread waits after uv_async_send until the main thread finishes ExecuteReadyHandles. This prevents a data race where the background thread re-enters rcl_wait (holding subscription references) while the main thread modifies the content filter. Use RAII guard to ensure notification on exception. - types/action_client.d.ts: Add enableFeedbackMsgOptimization option. - test/test-action-client.js: 7 new tests covering default/enable flag, normal feedback, selective feedback, cancel+new goal, multiple goals, and >6 goals overflow with auto-disable.

Copilot AI review requested due to automatic review settings March 30, 2026 04:42

Copilot started reviewing on behalf of minggangw March 30, 2026 04:43 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

minggangw force-pushed the fix-1456 branch from f03d66b to 1974819 Compare March 30, 2026 05:24

minggangw requested a review from Copilot March 30, 2026 05:27

Copilot started reviewing on behalf of minggangw March 30, 2026 05:27 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

minggangw requested a review from Copilot March 30, 2026 06:03

Copilot started reviewing on behalf of minggangw March 30, 2026 06:03 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

minggangw force-pushed the fix-1456 branch from e5f93e0 to b9a0540 Compare March 30, 2026 10:25

minggangw requested a review from Copilot March 31, 2026 03:20

Copilot started reviewing on behalf of minggangw March 31, 2026 03:20 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

minggangw force-pushed the fix-1456 branch from 33bc005 to a4f7a58 Compare March 31, 2026 06:03

minggangw added 2 commits March 31, 2026 15:52

Addresss comments

91299b4

Add typescript test

d3e4283

	typeof rclnodejs.actionConfigureFeedbackSubFilterAddGoalId === 'function';
	typeof rclnodejs.actionConfigureFeedbackSubFilterAddGoalId === 'function' &&
	typeof rclnodejs.actionConfigureFeedbackSubFilterRemoveGoalId === 'function';

-      this._feedbackSubFilterRemoveGoalId(goalHandle.goalId);
+      // Ensure feedback subscription filter removal is idempotent for this goal ID.
+      if (!this._removedFeedbackGoalIds) {
+        this._removedFeedbackGoalIds = new Set();
+      }
+      if (!this._removedFeedbackGoalIds.has(goalHandle.goalId)) {
+        this._feedbackSubFilterRemoveGoalId(goalHandle.goalId);
+        this._removedFeedbackGoalIds.add(goalHandle.goalId);
+      }

	this._feedbackSubFilterRemoveGoalId(
	statusMessage.goal_info.goal_id
	);

-  });
+  });
+  it('Subscription.isContentFilterSupported matches expected capabilities', function () {
+    const Subscription = rclnodejs.Subscription;
+    const supported = Subscription.isContentFilterSupported();
+    const expected = isContentFilteringSupported();
+    assert.strictEqual(typeof supported, 'boolean');
+    assert.strictEqual(supported, expected);
+  });

Conversation

minggangw commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

minggangw commented Mar 30, 2026 •

edited

Loading

coveralls commented Mar 30, 2026 •

edited

Loading