Repository navigation
Speed up built-in SEDP repair and increase discovery cache capacity - #449
Open
johannesschrimpf wants to merge 2 commits into
Open
johannesschrimpf wants to merge 2 commits into
johannesschrimpf wants to merge 2 commits into
Conversation
A reliable Writer answers an ACKNACK in handle_repair_data_send_worker(), which sends one change per call: the reader proxy's first unsent one. The rest of the request is left to the SendRepairData timer. A user writer schedules it nack_response_delay after the ACKNACK and re-arms it after a fifth of its Deadline period (or of 1 ms) while the reader proxy is in repair mode. For built-in writers that loop never ran. Since 21c5245 (0.14.0) they repair synchronously on the ACKNACK, so that a CPU-starved timer cannot delay discovery, and call handle_repair_data_send() once without scheduling SendRepairData. Before that they went through the timer, but the built-in QoS has an infinite Deadline, so the re-arm was due in about 13.6 years. Either way a built-in writer sends one requested sample per ACKNACK. The others wait for the next HEARTBEAT (every 100 ms while a reader is behind) and the ACKNACK it prompts, so a remote participant receives an SEDP history of N samples in N such rounds. Endpoint announcements that reach the remote reader meanwhile cannot be read until the repair has filled the gap before them: a reliable DataReader reads each writer's samples in sequence number order. Send the whole request on the ACKNACK: handle_repair_data_send() now calls the worker until the reader proxy leaves repair mode, at most BUILTIN_REPAIR_MAX_SAMPLES_PER_ROUND (256) times for built-in writers and once, as before, for user writers. 256 is the most sequence numbers one ACKNACK can request; the cap keeps a reader that is owed more from holding the event loop. A reader still in repair mode after the round gets the SendRepairData continuation that user writers use. Built-in writers schedule it after 0.2 ms, the user writers' delay without a Deadline, instead of a fifth of their infinite Deadline. User writers behave as before. Add unit tests that run a Writer without an event loop against a matched reader without locators: a built-in writer answers an ACKNACK for 40 samples in full (before: 39 left unsent); a built-in writer owed 266 samples sends 256 on the ACKNACK and the rest on the scheduled SendRepairData (before: 265 left after the ACKNACK); a user writer still sends one sample per SendRepairData event. Validation on macOS: - cargo test --lib, and with --features security (--test-threads=1) - cargo clippy --all-targets, with and without --features security - cargo +nightly fmt --check Assisted-by: Claude Fable 5.1 Assisted-by: OpenAI Codex (gpt-6-astra)
johannesschrimpf
changed the title
The built-in Subscriber QoS has no ResourceLimits (the spec gives LENGTH_UNLIMITED; this was a TODO), so the DDSCache keeps at most 64 samples per SEDP topic, counted over all writers of that topic. Its garbage collection runs every CACHE_CLEAN_PERIOD (4 s) and on some inserts, and removes the oldest received samples above the limit whether or not a reader has read them. A reliable DataReader reads each remote writer's samples in sequence number order (TopicCache::get_changes_in_range_reliable), so an SEDP sample that arrives while an earlier sample of the same writer is missing waits in the cache until the repair arrives. If 64 newer SEDP samples from any participants arrive meanwhile, the waiting sample is collected unread. The RTPS Reader has recorded it as received, so normal reliable retransmission does not recover it, and the endpoint it announces can remain undiscovered until it is announced again with a new sequence number. The previous commit shortens the repair; this one lets a waiting sample outlast many more announcements. Both matter when many participants exchange endpoints at once. This raises the limit rather than removing the eviction: the same loss remains possible once more than 4096 newer samples arrive while a gap is open. Give the DataReaders of DCPSPublication, DCPSSubscription, DCPSTopic and the secure publication and subscription topics their own QoS, builtin_sedp_reader_qos(): the built-in Subscriber QoS with ResourceLimits of 4096 samples. The DataReader's QoS raises the topic cache's limit. The Topics and DataWriters keep the built-in QoS without ResourceLimits, so the SEDP writers' history and send window do not change. The discovery setup macro gets a reader QoS argument for this. The SPDP, participant message and other secure built-in readers keep the default: their samples repeat periodically, so their caches would fill up to any limit. Why 4096: the cache holds samples up to the limit whether or not they have been read, so the limit is also its size once that many SEDP samples have arrived. SEDP samples are a few hundred bytes, so a full cache takes about 1 to 3 MiB per SEDP topic and DomainParticipant. max_instances is not enforced, and the DataReader keeps History depth (4) samples per instance, so the other two limits are 4096 as well to stay consistent. Add a TopicCache test: sample 2 of one writer waits for its sample 1 while 64 announcements of other participants arrive and are read, the cache is collected, then sample 1 arrives. With the built-in Subscriber QoS only sample 1 can then be read; with builtin_sedp_reader_qos() both can. Add a test that a DomainParticipant's SEDP topic caches get the 4096 limit and its SPDP topic cache keeps 64. With builtin_sedp_reader_qos() returning the QoS without ResourceLimits, both tests fail. Validation on macOS: - cargo test --no-fail-fast -- --test-threads=1, with and without --features security - cargo test --lib, with and without --features security, also with the default test threads - cargo clippy --all-targets, with and without --features security - cargo +nightly fmt --check Assisted-by: Claude Fable 5.1 Assisted-by: OpenAI Codex (gpt-6-astra)
johannesschrimpf
force-pushed
the
fix/sedp-repair-drain
branch
from
October 12, 2026 13:15
e7ababb to
e81a9ba
Compare
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
1 participant
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.
A peer's endpoint announcements (SEDP samples) written before a match reach a
participant by repair, and built-in writers answer each ACKNACK with one
sample, so a history of N announcements takes N HEARTBEAT rounds of 100 ms or
more. Announcements written after the match arrive live, but a reliable reader
cannot read them until the gap before them is filled, so they wait in the
receiver's SEDP topic cache. That cache keeps 64 samples across all
participants, and its garbage collection removes the waiting samples unread
when other participants announce many endpoints at the same time. The RTPS
reader has recorded the samples as received, so normal reliable
retransmission does not recover their loss from the discovery cache, and the
endpoints can remain undiscovered until a fresh announcement is processed.
Two commits, on master, independent of #447 (they merge cleanly):
handle_repair_data_sendtakes a budget: built-in writers answer the wholerequest, up to 256 samples (the most one ACKNACK can name), and a reader
still in repair mode gets the
SendRepairDatacontinuation that userwriters already use. User writers are unchanged (a test guards it). The
one-per-round behaviour was never chosen for discovery: it comes from
b9f5799 (2021, pacing for user data via a deadline/5 re-arm, which never
fires with the built-in writers' infinite Deadline); 21c5245 (0.14.0) made
the first repair synchronous and kept the single call.
builtin_sedp_reader_qos()is used by the DCPSPublication, DCPSSubscription,DCPSTopic and secure publication/subscription readers; it only raises the
topic cache limit, nothing changes on the wire. 4096 is 64 times the old
limit, about 2-4 MiB per SEDP topic and participant when full (estimate).
This is a mitigation, not a complete fix: the same loss remains possible
once more than 4096 newer samples arrive while a gap is open. The spec says
LENGTH_UNLIMITED; the structural alternative (never collect above a
reader's read position) needs reader positions in TopicCache.
Mechanism, at c076919:
src/rtps/writer.rs:1137-1145, 1196-1198, 1241-1243(synchronous built-in repair, one call),
1346-1355(worker sends onlyfirst_unsent_change),460-478(user writers re-arm);src/structure/dds_cache.rs:189-198(64 default),244-264and101-110(collection on SN%64 inserts and every 4 s),
410-458(oldest received first,read or not),
343-365(reliable reads stop at the gap);src/discovery/discovery.rs:1986-1994(the ResourceLimits TODO).Reproduction
Same harness as #447, each node its own process; a round fails when the
subscriber receives nothing within 10 s. "Before" is master + #447, "after"
adds this PR.
A 10-round flood batch with discovery and cache logging at debug shows the
mechanism: before, 6 of 10 rounds lost, each with the subscriber's
DCPSSubscription writer repairing 2-6 samples one per round while the
publisher's cache was collected 6 times in between; after, 0 of 10, every
repair round ending with nothing left unsent.
Caveats: the batches are not timing-matched (other builds ran on the host);
30 rounds cannot separate commit 2 from commit 1 (1/30 vs 0/30), but the one
commit-1-only failure has the eviction signature. Duplicate ACKNACKs (one per
writer locator) now repeat the whole repair instead of one sample; dropping
ACKNACKs with an already-seen count would remove that and is a separate
change. With the burstier repair, the event loop's 64-slot discovery command
channel dropped an endpoint announcement in 2 of 30 flood rounds (3
announcements; 0 before), and the separate reader/writer removal channels
overflowed at teardown in more rounds; those endpoints can stay unannounced.
#450 fixes that drop path and a similar one for ACKNACKs, and
should land before or together with this one. As in #447, I have not shown
which mechanism caused the original case on the robot.
Tests
Deterministic. The three writer tests (
builtin_writer_repairs_whole_acknack,builtin_writer_repair_is_capped_per_round_and_continues,user_writer_repairs_one_sample_per_timer_event) bind one ephemeral UDPsocket and send nothing;
sedp_reader_limits_keep_sample_waiting_for_repairdrives a TopicCache and calls the collection directly;
sedp_topic_caches_get_sedp_reader_limitscreates a participant on domain 0and reads its caches' limits. The four fix tests fail with their commit
reverted; the user-writer test is a compatibility guard that passes before
and after.
Validation (macOS)
cargo test --no-fail-fast -- --test-threads=1with and without--features security(698 / 708 lib tests, integration tests, 58 doctests);cargo test --libwith default threads, plus 174 parallel runs across masterand these commits with no hangs;
cargo clippy --all-targets(both featuresets) clean;
cargo +nightly fmt --check. Not run locally: the 1.88 MSRVtoolchain (CI covers it).
Developed with assistance from Claude Fable 5.1 and OpenAI Codex (gpt-6-astra).