From 9c67d1ec8ccb0bf2a813c5624313a0c498e950ec Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Fri, 27 Feb 2026 10:43:37 +0900 Subject: [PATCH 1/3] avoid stale parameter events in content filter tests. Signed-off-by: Tomoya Fujita --- .../rclcpp/test_parameter_event_handler.cpp | 92 ++++++++++++++----- 1 file changed, 67 insertions(+), 25 deletions(-) diff --git a/rclcpp/test/rclcpp/test_parameter_event_handler.cpp b/rclcpp/test/rclcpp/test_parameter_event_handler.cpp index 3e14632c35..3f23421440 100644 --- a/rclcpp/test/rclcpp/test_parameter_event_handler.cpp +++ b/rclcpp/test/rclcpp/test_parameter_event_handler.cpp @@ -471,14 +471,38 @@ TEST_F(TestNode, ConfigureNodesFilterAndCheckAddParameterEventCallback) std::atomic_bool received_from_remote_node1{false}; std::atomic_bool received_from_remote_node2{false}; + // The ParameterEventHandler creates its /parameter_events subscription (with no + // content filter) at construction time. When declare_parameter is later called on + // remote nodes, it internally triggers set_parameter which publishes a ParameterEvent + // with the parameter in the new_parameters field. That event arrives on the + // already-existing, unfiltered subscription and is stored in its queue. + // When configure_nodes_filter is subsequently called, it applies a content filter to + // the subscription, but events already sitting in the subscriber's queue are not + // affected. Per the DDS specification, a content-filtered topic filter applies at the + // point of data delivery/insertion into the reader's cache. Once data is in the cache, + // it has been "received." Retroactively purging it when the filter changes is not + // standard behavior. + // rmw_connextdds correctly honors this: only future incoming samples are filtered, so + // queued declaration events would still be delivered. rmw_fastrtps_cpp may retroactively + // apply filters to already-queued messages or deliver them lazily, causing those + // events to be silently discarded - which is arguably incorrect but happens to make + // naive tests pass. + // Only track changed_parameters to ignore queued declare_parameter events. auto cb = - [&remote_node_name1, &remote_node_name2, &received_from_remote_node1, - &received_from_remote_node2] + [&remote_node_name1, &remote_node_name2, + &remote_node1_param_name, &remote_node2_param_name, + &received_from_remote_node1, &received_from_remote_node2] (const rcl_interfaces::msg::ParameterEvent & parm) { - if (parm.node == "/" + remote_node_name1) { - received_from_remote_node1 = true; - } else if (parm.node == "/" + remote_node_name2) { - received_from_remote_node2 = true; + for (const auto & changed_parameter : parm.changed_parameters) { + if (parm.node == "/" + remote_node_name1 && + changed_parameter.name == remote_node1_param_name) + { + received_from_remote_node1 = true; + } else if (parm.node == "/" + remote_node_name2 && + changed_parameter.name == remote_node2_param_name) + { + received_from_remote_node2 = true; + } } }; @@ -505,7 +529,12 @@ TEST_F(TestNode, ConfigureNodesFilterAndCheckAddParameterEventCallback) }; { - std::thread thread(wait_param_event, 1s, nullptr); + std::thread thread( + wait_param_event, + 3s, + [&received_from_remote_node2]() { + return received_from_remote_node2.load(); + }); std::this_thread::sleep_for(100ms); remote_node1->set_parameter(rclcpp::Parameter(remote_node1_param_name, 20)); remote_node2->set_parameter(rclcpp::Parameter(remote_node2_param_name, "abc")); @@ -565,29 +594,37 @@ TEST_F(TestNode, ConfigureNodesFilterAndCheckAddParameterCallback) std::atomic_bool received_from_remote_node1{false}; std::atomic_bool received_from_remote_node2{false}; - auto cb_remote_node1 = - [&remote_node1_param_name, &received_from_remote_node1] - (const rclcpp::Parameter & parm) { - if (parm.get_name() == remote_node1_param_name) { - received_from_remote_node1 = true; - } - }; - - auto cb_remote_node2 = - [&remote_node2_param_name, &received_from_remote_node2] - (const rclcpp::Parameter & parm) { - if (parm.get_name() == remote_node2_param_name) { - received_from_remote_node2 = true; + // Only track changed_parameters to ignore queued declare_parameter events. + auto cb_changed_event = + [&remote_node_name1, &remote_node_name2, + &remote_node1_param_name, &remote_node2_param_name, + &received_from_remote_node1, &received_from_remote_node2] + (const rcl_interfaces::msg::ParameterEvent & event) { + for (const auto & changed_parameter : event.changed_parameters) { + if (event.node == "/" + remote_node_name1 && + changed_parameter.name == remote_node1_param_name) + { + received_from_remote_node1 = true; + } else if (event.node == "/" + remote_node_name2 && + changed_parameter.name == remote_node2_param_name) + { + received_from_remote_node2 = true; + } } }; // Configure to only receive parameter events from remote_node_name2 ASSERT_TRUE(param_handler->configure_nodes_filter({remote_node_name2})); - auto handler1 = param_handler->add_parameter_callback( - remote_node1_param_name, cb_remote_node1, remote_node_name1); - auto handler2 = param_handler->add_parameter_callback( - remote_node2_param_name, cb_remote_node2, remote_node_name2); + // Note: add_parameter_callback internally uses get_parameter_from_event, which + // searches both new_parameters and changed_parameters. This means a queued + // declare_parameter event (new_parameters) could trigger the parameter callback + // before the content filter takes effect — bypassing the filter on RMW + // implementations that don't retroactively purge queued messages. + // To make this test deterministic, we use an event-level callback that only + // inspects changed_parameters for the assertion flags, rather than relying on + // parameter-level callbacks which cannot distinguish new vs changed. + auto event_handler = param_handler->add_parameter_event_callback(cb_changed_event); rclcpp::executors::SingleThreadedExecutor executor; executor.add_node(node); @@ -607,7 +644,12 @@ TEST_F(TestNode, ConfigureNodesFilterAndCheckAddParameterCallback) }; { - std::thread thread(wait_param_event, 1s, nullptr); + std::thread thread( + wait_param_event, + 3s, + [&received_from_remote_node2]() { + return received_from_remote_node2.load(); + }); std::this_thread::sleep_for(100ms); remote_node1->set_parameter(rclcpp::Parameter(remote_node1_param_name, 20)); remote_node2->set_parameter(rclcpp::Parameter(remote_node2_param_name, "abc")); From 1040e47fc205eee01e0488154a7c54a570a087d3 Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Mon, 2 Mar 2026 09:10:46 +0900 Subject: [PATCH 2/3] honor the test purpose to call add_parameter_callback(). Signed-off-by: Tomoya Fujita --- .../rclcpp/test_parameter_event_handler.cpp | 45 ++++++++----------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/rclcpp/test/rclcpp/test_parameter_event_handler.cpp b/rclcpp/test/rclcpp/test_parameter_event_handler.cpp index 3f23421440..36c03b4bba 100644 --- a/rclcpp/test/rclcpp/test_parameter_event_handler.cpp +++ b/rclcpp/test/rclcpp/test_parameter_event_handler.cpp @@ -591,40 +591,33 @@ TEST_F(TestNode, ConfigureNodesFilterAndCheckAddParameterCallback) remote_node1->declare_parameter(remote_node1_param_name, 10); remote_node2->declare_parameter(remote_node2_param_name, "Default"); + // The ParameterEventHandler creates its /parameter_events subscription (with no + // content filter) at construction time. When declare_parameter is later called on + // remote nodes, it internally triggers set_parameter which publishes a ParameterEvent + // with the parameter in the new_parameters field. That event arrives on the + // already-existing, unfiltered subscription and is stored in its queue. + // Recreate the param_handler so its new subscription starts with an empty queue, + // avoiding interference from queued declare_parameter events. + param_handler.reset(); + param_handler = std::make_shared(node); + std::atomic_bool received_from_remote_node1{false}; std::atomic_bool received_from_remote_node2{false}; - // Only track changed_parameters to ignore queued declare_parameter events. - auto cb_changed_event = - [&remote_node_name1, &remote_node_name2, - &remote_node1_param_name, &remote_node2_param_name, - &received_from_remote_node1, &received_from_remote_node2] - (const rcl_interfaces::msg::ParameterEvent & event) { - for (const auto & changed_parameter : event.changed_parameters) { - if (event.node == "/" + remote_node_name1 && - changed_parameter.name == remote_node1_param_name) - { - received_from_remote_node1 = true; - } else if (event.node == "/" + remote_node_name2 && - changed_parameter.name == remote_node2_param_name) - { - received_from_remote_node2 = true; - } - } + auto cb1 = [&received_from_remote_node1](const rclcpp::Parameter &) { + received_from_remote_node1 = true; + }; + auto cb2 = [&received_from_remote_node2](const rclcpp::Parameter &) { + received_from_remote_node2 = true; }; // Configure to only receive parameter events from remote_node_name2 ASSERT_TRUE(param_handler->configure_nodes_filter({remote_node_name2})); - // Note: add_parameter_callback internally uses get_parameter_from_event, which - // searches both new_parameters and changed_parameters. This means a queued - // declare_parameter event (new_parameters) could trigger the parameter callback - // before the content filter takes effect — bypassing the filter on RMW - // implementations that don't retroactively purge queued messages. - // To make this test deterministic, we use an event-level callback that only - // inspects changed_parameters for the assertion flags, rather than relying on - // parameter-level callbacks which cannot distinguish new vs changed. - auto event_handler = param_handler->add_parameter_event_callback(cb_changed_event); + auto h1 = param_handler->add_parameter_callback( + remote_node1_param_name, cb1, "/" + remote_node_name1); + auto h2 = param_handler->add_parameter_callback( + remote_node2_param_name, cb2, "/" + remote_node_name2); rclcpp::executors::SingleThreadedExecutor executor; executor.add_node(node); From f81117fe3bea6502e7740fad02db82acaeaa2136 Mon Sep 17 00:00:00 2001 From: Tomoya Fujita Date: Mon, 2 Mar 2026 09:29:29 +0900 Subject: [PATCH 3/3] make cpplint happy. Signed-off-by: Tomoya Fujita --- rclcpp/test/rclcpp/test_parameter_event_handler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/test/rclcpp/test_parameter_event_handler.cpp b/rclcpp/test/rclcpp/test_parameter_event_handler.cpp index 36c03b4bba..149ba533c3 100644 --- a/rclcpp/test/rclcpp/test_parameter_event_handler.cpp +++ b/rclcpp/test/rclcpp/test_parameter_event_handler.cpp @@ -498,7 +498,7 @@ TEST_F(TestNode, ConfigureNodesFilterAndCheckAddParameterEventCallback) changed_parameter.name == remote_node1_param_name) { received_from_remote_node1 = true; - } else if (parm.node == "/" + remote_node_name2 && + } else if (parm.node == "/" + remote_node_name2 && // NOLINT(readability/braces) changed_parameter.name == remote_node2_param_name) { received_from_remote_node2 = true;