feat: introduce the advanced publisher and subscriber #368

YuanYuYuan · 2024-12-31T11:18:30Z

With this PR, we replace the QueryingSubscriber and PublicationCache (+ a normal publisher)
with the AdvancedSubscriber and AdvancedPublisher.

NOTE: A deadlock is found when undeclaring advanced subscribers. It will be fixed once eclipse-zenoh/zenoh#1685 is merged and synced with zenoh-c.

YuanYuYuan · 2025-01-07T07:58:06Z

Update: The deadlock fix has been merged. Mark this PR as ready for review.

YuanYuYuan · 2025-01-08T09:32:53Z

Test Failure

ros2.repos

rclcpp_action
- 1 - test_client
rclc_parameter
- 1 - rclc_parameter_test
test_rclcpp.log
- 74 - test_n_nodes__rmw_zenoh_cpp (Failed) <--- Flaky

No new failure is found.

YuanYuYuan · 2025-01-08T15:04:04Z

Update: bump up zenoh-cpp version to include the memory leak fix eclipse-zenoh/zenoh-cpp#363.

YuanYuYuan · 2025-01-10T01:42:19Z

Will rebase once #405 is merged.

Yadunund

Please see feedback inline.
I have the following additional questions

It looks like this PR does not fix [Test Failure] rclcpp_action / test_client #326. But from my experience, it changes the behavior from deterministically failing to failing sometimes. ie, the test is now flaky. Do you observe the same?
With AdvancedSubscribers do we no longer need to call Get() when we detect an AdvancedPublisher with publication cache? This was needed to fix Subscribers sometimes not getting messages from topics using durability transient_local #263.

Yadunund · 2025-01-11T22:23:19Z

zenoh_cpp_vendor/CMakeLists.txt

@@ -22,7 +22,7 @@ set(ZENOHC_CARGO_FLAGS "--no-default-features$<SEMICOLON>--features=shared-memor
 # - https://github.com/eclipse-zenoh/zenoh/pull/1696 (Fix SHM Garbage Collection (GC) policy)
 ament_vendor(zenoh_c_vendor
  VCS_URL https://github.com/eclipse-zenoh/zenoh-c.git
-  VCS_VERSION 61d8fcc136ce4ed36d921a32244da4f3d81a6097
+  VCS_VERSION 85ca060fa4037239ca4102a3a61f96626cc6b434


Please replace the comment above and below to include changes in this commit hash.

Thanks. This was out of sync. We don't need to change it in this PR.

Yadunund · 2025-01-11T23:11:43Z

rmw_zenoh_cpp/src/detail/graph_cache.hpp

@@ -189,15 +189,6 @@ class GraphCache final
  /// Returns true if the entity is a publisher or client. False otherwise.
  static bool is_entity_pub(const liveliness::Entity & entity);

-  void set_querying_subscriber_callback(


You'll also need to delete querying_subs_cbs_ and update graph_cache.cpp to not iterate over querying_subs_cbs_

Done in 9cc776b

Yadunund · 2025-01-11T23:18:58Z

rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp

-  zenoh::KeyExpr pub_ke(entity->topic_info()->topic_keyexpr_);
+  auto adv_pub_opts = zenoh::ext::SessionExt::AdvancedPublisherOptions::create_default();
+  adv_pub_opts.publisher_detection = true;
+  adv_pub_opts.sample_miss_detection = true;


Looking at the documentation for this option, it sounds like this should be set true only if adapted_qos_profile.reliability == RMW_QOS_POLICY_RELIABILITY_RELIABLE? Could you clarify how this differs from setting pub_opts.reliability = Z_RELIABILITY_RELIABLE?

The reliability option can still be set by the normal publisher option. See here. The reliability in zenoh is based on TCP reliability per see. The sample_miss_detection here is a custom data retransmission feature cached on the publisher side in the sense of transient_local duriability.

Yadunund · 2025-01-11T23:47:55Z

rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp

-    pub_cache_opts.history = adapted_qos_profile.depth;
-    pub_cache_opts.queryable_complete = true;
-
-    std::string queryable_prefix = entity->zid();
-    pub_cache_opts.queryable_prefix = zenoh::KeyExpr(queryable_prefix);
-
-    pub_cache = session->ext().declare_publication_cache(
-      pub_ke, std::move(pub_cache_opts), &result);


We previously needed to do this to fix #263. Can you confirm that we no longer need to explicitly call QueryingSubscriber::get() when we discover an AdvancedPublisher in the graph?

We should no longer need this hack. Otherwise, it's a bug in zenoh.

Yadunund · 2025-01-11T23:50:49Z

rmw_zenoh_cpp/src/detail/rmw_subscription_data.cpp

+    adv_sub_opts.history->detect_late_publishers = true;
+    adv_sub_opts.history->max_samples = entity_->topic_info()->qos_.depth;
+    adv_sub_opts.recovery = zenoh::ext::SessionExt::AdvancedSubscriberOptions::RecoveryOptions{};
+    adv_sub_opts.recovery->periodic_queries_period_ms = 1000;


Again looking at the documentation, it sounds like recovery options should only be enabled if reliability is RELIABLE. Could you clarify on the behavior?

Similarly, answered in #368 (comment).

…claration deadlock

…lipse-zenoh/zenoh-cpp#363

YuanYuYuan · 2025-01-13T08:46:04Z

Please see feedback inline. I have the following additional questions

1. It looks like this PR does not fix [[Test Failure] rclcpp_action / test_client #326](https://github.com/ros2/rmw_zenoh/issues/326). But from my experience, it changes the behavior from deterministically failing to failing sometimes. ie, the test is now flaky. Do you observe the same?

2. With `AdvancedSubscribers` do we no longer need to call `Get()`  when we detect an `AdvancedPublisher` with publication cache? This was needed to fix [Subscribers sometimes not getting messages from topics using durability transient_local #263](https://github.com/ros2/rmw_zenoh/issues/263).

Here’s an improved version of your paragraph with clearer phrasing and a more professional tone:

This PR does not resolve the issue reported in [Test Failure] rclcpp_action / test_client #326. Based on the investigation, the root cause lies in the inappropriate synchronization assumptions in the test. We have also observed similar failures (slightly more often) when using the previous QueryingSubscriber implementation. To address this issue, we can either make the local publisher-to-subscriber action a blocking call or modify the test itself.
We expect the issue will be resolved. So no longer need the hack 😄

YuanYuYuan · 2025-01-13T08:48:28Z

Redid the test with the repos. No new failures were found.

rclcpp_action/test_client
rclc_parameter/rclc_parameter_test

YuanYuYuan mentioned this pull request Dec 31, 2024

[Test Failure] rclcpp_action / test_client #326

Open

clalancette marked this pull request as draft January 2, 2025 16:09

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from e0c490f to 1f9357f Compare January 7, 2025 07:57

YuanYuYuan marked this pull request as ready for review January 7, 2025 07:58

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from 08b0906 to b76ee4c Compare January 11, 2025 17:05

Yadunund requested changes Jan 11, 2025

View reviewed changes

YuanYuYuan added 8 commits January 13, 2025 16:06

feat: introduce the advanced publisher/subscriber

b8ce3c6

chore: ament_uncrustify and ament_cpplint

ab081f2

deps(zenoh): bump up zenoh-c and zenoh-cpp to include the fix of unde…

05228d0

…claration deadlock

deps(zenoh): bumup zenoh-cpp to include the fix to the memory leak ec…

0b51b3f

…lipse-zenoh/zenoh-cpp#363

trigger CI

25c6d55

fix: resolve out-of-sync

a31fd16

refactor: remove the unneeded querying callbacks

448bb6c

fix: reorder the advanced pub/sub options to match the ROS2 QoS

9cc776b

YuanYuYuan force-pushed the feat/advanced-pub-sub branch from b76ee4c to 9cc776b Compare January 13, 2025 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce the advanced publisher and subscriber #368

feat: introduce the advanced publisher and subscriber #368

YuanYuYuan commented Dec 31, 2024

YuanYuYuan commented Jan 7, 2025

YuanYuYuan commented Jan 8, 2025 •

edited

Loading

YuanYuYuan commented Jan 8, 2025

YuanYuYuan commented Jan 10, 2025

Yadunund left a comment

Yadunund Jan 11, 2025

YuanYuYuan Jan 13, 2025

Yadunund Jan 11, 2025

YuanYuYuan Jan 13, 2025

Yadunund Jan 11, 2025

YuanYuYuan Jan 13, 2025

Yadunund Jan 11, 2025

YuanYuYuan Jan 13, 2025

Yadunund Jan 11, 2025

YuanYuYuan Jan 13, 2025

YuanYuYuan commented Jan 13, 2025

YuanYuYuan commented Jan 13, 2025 •

edited

Loading

feat: introduce the advanced publisher and subscriber #368

Are you sure you want to change the base?

feat: introduce the advanced publisher and subscriber #368

Conversation

YuanYuYuan commented Dec 31, 2024

YuanYuYuan commented Jan 7, 2025

YuanYuYuan commented Jan 8, 2025 • edited Loading

Test Failure

YuanYuYuan commented Jan 8, 2025

YuanYuYuan commented Jan 10, 2025

Yadunund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuanYuYuan commented Jan 13, 2025

YuanYuYuan commented Jan 13, 2025 • edited Loading

YuanYuYuan commented Jan 8, 2025 •

edited

Loading

YuanYuYuan commented Jan 13, 2025 •

edited

Loading