Repository navigation
feat(hiroz)!: let a raw sub join the cpu group - #385
Merged
Merged
Conversation
The three mutant tests failed through a plain assert. They now fail through r1_assert with their own name, so a failure carries the R1ASSERT marker.
A joined typed subscriber that is dropped must stop its callback on the group key. No test owned the mutant that forgets the group subscription on drop. The callback runs on the group key before the drop, then ten puts after it add none.
A rule on the group key stops a joined subscriber from receiving buffer messages only. A message without a buffer arrives on the base key. Off avoids the join but does not restore buffer messages from a released publisher.
A raw subscriber that joins the group owns three handles: the token, the base subscription and the group subscription. Tearing down by hand must remove all three while the binding is still in scope. The test tears down through the public inner field, so it is red until the wrapper has a consuming teardown.
RawSubscriber made inner public next to the token and the group subscription. A caller that undeclared inner stopped the base subscription only: the group key kept calling back and the token stayed visible. The handles are private now, and undeclare consumes the wrapper: token first, then both subscriptions. The teardown test calls it.
create_raw_subscriber_inner only forwarded from create_raw_subscriber_with_group. Its body now lives in the public constructor, and the two legacy constructors call that constructor directly.
Each raw test built its own router, plain session, context and node, and repeated the token, delivery and teardown checks. A fixture and four assertion helpers now carry them. The tests that said opt-in say explicit Cpu selection, because joining is the default. Every policy case, QoS and key format exclusion, ACL control, exact-key check and lifecycle check stays.
The mixed native cell has a released subscriber next to the raw ones, and that subscriber can make the publisher write the group key by itself. The new cell has the default raw subscriber alone. It requires one matched subscriber in the publisher report, every sample on the group key and none on the base key, and every sample received once. The two cells share one fixture and the same checks. This cell needs a buffer-aware rmw_zenoh_cpp, so only the Lyrical interop job runs it.
The typed suite declared a peer session, a node and 30 matching listeners inline to prove that the group subscription exists when the token shows. The check now lives in the shared test helpers and takes the subscriber builder, so the raw suite can run it too. The typed test calls it and asserts the same thing.
The raw suite checked that the token shows after the subscription only through an ACL wait. It had no test that a publisher reading the token finds the group subscription. The raw path declares both subscriptions before the token, and a change that declares the token first now fails this test.
Clippy with the raw-sub-tests and ros-interop features flags the borrow of a formatted node name in the sole raw interop cell. The CI clippy step does not enable raw-sub-tests, so it never lints this file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A buffer-aware
rmw_zenoh_cpppublisher stops writing the plain topic key for a message with an unboundeduint8[]orchar[]field. It writes<topic key>/_buf_cpufor subscribers whose liveliness token ends withbackends:cpu:. See ros2/rmw_zenoh#930. This text calls<topic key>the base key and<topic key>/_buf_cputhe group key.A raw subscriber declared no token and read only the base key. It received nothing from such a publisher, with no error and no log. This is the report in #369.
Part of #372.
What changes
Autojoins it, as it joins a typed subscriber without a schema.create_raw_subscriberandcreate_raw_subscriber_with_qosuseAuto. The C and Go bindings call them, so they join too.ZNode::create_raw_subscriber_with_group, only with theffifeature. It takesAuto,CpuorOff.Offstays out and declares no token. The Go bindings have no entry point for it yet.backends:cpu:. One callback serves both keys.RawSubscriberhas no public field now.RawSubscriber::undeclareconsumes the wrapper and stops the token and both subscriptions. Dropping the wrapper stops the same three handles in the same order.backends:field.cargo nextest run --no-tests=failstep. Without theraw-sub-testsfeature the suite compiles to nothing, and the step fails instead of passing with zero tests.Before and after
backends:cpu:Offraw.inner.undeclare()(featureffi)raw.undeclare()Compatibility
RawSubscriber.inneris no longer publicffifeature that callsraw.inner.undeclare()(the field is public in release 0.2.0)raw.undeclare()create_raw_subscriber_with_groupwithOffto stay out (Rust only)A router that allows only the base key starves a joined raw, C or Go subscriber of a buffer-aware publisher, with no error. Allow both keys.
BufferGroup::Offneeds no group key. A subscriber with nobackends:field gets no buffer message from a buffer-aware publisher. This holds until a release carries ros2/rmw_zenoh#1078, soOffdoes not bring those messages back.Design
A raw subscriber has no message schema, so
AutoandCpudo the same for it. A subscriber with no schema joins on every topic, not only on buffer topics. The subscriptions come before the token, as in #384. The token drops first. This sets the order of local undeclaration only. A remote publisher can still count the subscriber, and send to it, for a short time after the drop. The typed subscriber and the cache drop their handles in a different order; this PR leaves them unchanged.Tests
The raw suite plays the publisher with a plain Zenoh session, so it needs no ROS installation. CI runs it in its own
cargo nextest run --no-tests=failstep, so a build without theraw-sub-testsfeature fails instead of passing with zero tests.buffer_cpu_group_raw.rs)raw_subscriber_default_joins_and_announces_backendsbackends:cpu:token, and one callback serves both keysraw_subscribers_that_stay_out_announce_no_backends_and_skip_the_group_keyOff, transient-local QoS and a custom key format declare no token and read only the base keyraw_action_client_feedback_subscriber_stays_outraw_subscriber_declares_two_exact_subscriptions_and_off_declares_oneOffraw_group_subscription_is_declared_before_the_token_is_visibleraw_access_control_starves_the_group_key_silentlyraw_denied_group_key_leaves_the_base_key_flowingraw_create_and_teardown_cycles_leave_no_token_and_no_callbackundeclare()and drop remove the token and both callbacksraw_join_logs_both_keys_at_debug_leveldebugline with both keys, and anOffsubscriber logs nonebuffer_cpu_group_raw_interop.rs)raw_default_subscriber_receives_from_a_released_publisherrmw_zenoh_cpppublisher, beside a released subscriber and anOffraw subscriber; theOffsubscriber receives none. The released subscriber can make the publisher write the group key alone, so this cell does not show that the raw token matchedraw_default_subscriber_alone_matches_a_released_publisherThe interop cells publish with
ros2 topic puband count what arrives. They check totals, not per-message identity.To check that the tests can fail, I added one deliberate bug at a time to the production code. Each bug turned at least one raw test red.
undeclare()leaves thebackends:cpu:token declaredundeclare()leaves the group-key callback runningcreate_raw_subscriberor_with_qospassOffOff, transient-local, custom key format or the feedback casedebugline for a raw, C or Go join#384 has merged. Merge order: this PR, #388, #389, then #386.