Repository navigation
Do not drop endpoint announcements or ACKNACKs on full channels - #450
Open
johannesschrimpf wants to merge 2 commits into
Open
johannesschrimpf wants to merge 2 commits into
johannesschrimpf wants to merge 2 commits into
Conversation
The DP event loop tells Discovery about each new local reader and writer with an AddLocalReader/AddLocalWriter command on Discovery's command channel. That channel is bounded (64 slots), it also carries the AddTopic commands of create_datareader/create_datawriter, and only the Discovery thread drains it. The event loop used try_send and, when the channel was full, logged an error and dropped the command. Discovery writes an endpoint's DCPSPublication or DCPSSubscription sample only on that command, so the endpoint was never announced and no remote participant ever matched it. Nothing retried, and reliability does not notice: the SEDP writer's HEARTBEATs advertise a complete history without the sample. The channel fills when an application creates many endpoints back to back (two commands each, e.g. a ROS 2 node and its parameter services) while Discovery is busy, for example processing the SEDP samples of other participants or waiting for room in its notification channel to the event loop. A blocking send is not an option: Discovery blocks on that bounded notification channel when it is full, and only the event loop drains it, so the two threads could wait for each other. Instead, the event loop keeps the commands that do not fit in a queue, in order, and retries them on every loop iteration. While the queue is not empty, the loop polls with the 2 ms timeout it already uses for pending outbound work, because nothing wakes it when Discovery drains its channel. Each queued command is logged at debug level. When commands have waited for 5 s, the event loop warns once, so that a Discovery that has stopped taking commands shows in the log, and it logs at info level when Discovery has taken them. A command is now dropped only when Discovery has stopped, which is normal during shutdown, so that is logged at info level. Add a regression test: it gives a DPEventLoop a Discovery command channel with room for one command and adds three local readers on the test thread, as the event loop does on their event, so that all three commands have been tried before anything drains the channel. Then it runs the event loop while another thread drains the channel as Discovery does, and asserts that Discovery is told about all three readers, in order. Without the fix it fails (5 of 5 runs), because only the first reader's command arrives; with it, it passes (5 of 5). Validation on macOS: - cargo test --lib - cargo test --no-fail-fast -- --test-threads=1, with and without --features security - 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)
The message receiver forwards each ACKNACK and NACK_FRAG to the DP event loop, which hands it to the local writer. Both run on the event loop thread, so the forwarding used try_send on a 100-slot channel, and when the channel was full it discarded the submessage with an info log line. The event loop drains that channel only when it handles the channel's readiness event. Until then each UDP listener can deliver up to MAX_LISTENER_MESSAGES_PER_POLL (16) datagrams per poll, and a datagram can carry several ACKNACKs. A burst of more than 100, e.g. a crowd of remote readers answering the HEARTBEATs of the built-in SEDP writers at the same time, lost the rest. A reader whose ACKNACK was discarded gets no repair until the writer's next HEARTBEAT and the reader's next ACKNACK, so each discard costs that reader a whole HEARTBEAT round, and repeated discards stretch discovery over many rounds. Back-pressure is not possible on this channel: sender and receiver are the same thread, so a blocking send would deadlock. Use an unbounded channel instead. Its backlog is limited by what the event loop reads between two drains: the listeners read at most MAX_LISTENER_MESSAGES_PER_POLL datagrams each per poll and leave the rest in the socket buffers, and the event loop drains the whole channel each time it becomes readable. Add a regression test: the message receiver of a DPEventLoop gets 200 ACKNACK datagrams from 200 remote readers before the event loop drains the pipe, and the test asserts that all 200 ACKNACKs come out of it. Without the fix it fails (5 of 5 runs, 100 discarded each); with it, it passes (5 of 5). Validation on macOS: - cargo test --lib - cargo test --no-fail-fast -- --test-threads=1, with and without --features security - 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)
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.
Two internal channels of a DomainParticipant drop a message when they are
full:
over Discovery's 64-slot command channel with
try_send(
src/rtps/dp_event_loop.rs:1316-1331at c076919). When the channel isfull, the command is dropped with an error log, and nothing sends it
again. Discovery writes an endpoint's SEDP sample only on that command
(
discovery.rs:759-764, 1830-1876), so the endpoint is never announcedand no remote participant matches it, indefinitely; reliability cannot
notice, because the sample never existed. The channel fills when an
application creates many endpoints back to back (about two commands each;
the AddTopic of
create_datareader/writeruses it too) while Discoveryis busy with other participants' SEDP samples or waiting for room in its
own notification channel to the event loop.
over a 100-slot channel with
try_send(message_receiver.rs:862-892).When a burst fills it, the rest are discarded with an info log. The event
loop drains the channel only on its readiness event; until then each of
the four listeners can deliver up to 16 datagrams per poll, each carrying
several submessages, and readiness-event ordering decides when the drain
happens, so a crowd of readers answering the built-in writers' HEARTBEATs
at once can exceed the 100 slots. A reader whose ACKNACK is discarded can
wait until a subsequent HEARTBEAT prompts another one, so this loss delays
reliable repair rather than preventing discovery.
One commit per channel, on master, independent of #447 and #449 (touches
different files; cherry-picks onto both without conflicts). Those two PRs
address the other mechanisms found in the same investigation.
in a
VecDequeon the event loop, later commands queue behind it (orderkept), and the loop retries on every iteration, polling with its existing
2 ms timeout while anything waits. After 5 s of waiting it warns once and
logs when the queue drains again. A blocking send is not an option:
Discovery sends its notifications to the event loop with a blocking send
on a 32-slot channel that only the event loop drains, so two blocking
sends would let the threads wait for each other. This change adds no
blocking send; the three blocking
StartKeyExchangeWithRemote*sends ofthe
securityfeature are pre-existing and left for a follow-up. Acommand is dropped only when Discovery has exited, which on shutdown is
the normal order and is logged at info.
loop thread, so back-pressure would deadlock at once; the channel becomes
mio_extras::channel::channel()and forwarding usessend. What isqueued between two drains is what the listeners read in that time, at
most 16 datagrams per listener per poll; the per-poll limit bounds receive
work, not the exact number of queued ACKNACKs, and further datagrams wait
in the socket buffers.
Reproduction
Deterministic probes showed both drops at c076919 and on #449's branch: with
one free slot in the command channel, Discovery was told about only the first
of three new readers; with 101 ACKNACKs between two drains, one was
discarded. The two regression tests are trimmed versions of those probes.
The multi-process harness from #447 got a "burst" variant (two nodes create
40 publishers and 40 subscriptions each at start; a round fails unless all
160 endpoints match) and a 200-pair version. Before (#447 + #449) and after
(plus this PR): 0 unmatched endpoints in 4800 + 4000, crowd 30/30 both. The
harness did not reach the command-channel limit (an endpoint every 6-8 ms,
Discovery keeps up; the retry queue never engaged) and reached the ACKNACK
limit once (5 discards in 1 of 132 baseline rounds, every endpoint still
matched), so these runs show no harm rather than a fixed failure. The endpoint drop
was observed in runtime logs in #449's flood runs: bystanders creating 60
readers each logged "Failed to inform Discovery about the new endpoint" 3
times in 2 of 30 rounds. Timing differences between before and after were
within the spread of back-to-back control pairs.
Tests
In the tests module of
src/rtps/dp_event_loop.rs, each failing with itscommit's non-test changes reverted:
every_new_local_endpoint_is_announced_to_discovery: an event loop with aone-slot command channel has three new readers added on the test thread,
so all three sends are attempted before anything drains; then the real
event loop runs while a thread drains the channel as Discovery does. All
three
AddLocalReadercommands must arrive in order. Fails withoutcommit 1 (only the first arrives), passes with it.
acknack_burst_reaches_writers: 200 ACKNACK datagrams from 200 readersbefore a drain; all 200 must come out of the channel towards the writers
(it verifies delivery to the writer-dispatch queue, not the writers'
processing). Fails without commit 2 (100 discarded), passes with it.
Not covered here
AddTopicis stilltry_sendunder the DiscoveryDB lock (only theDCPSTopic sample is lost; matching does not use it); the security feature's
blocking sends; endpoint removal's
try_send_timeouton 4-slot channels;a queued add overtaken by a remove of the same endpoint (same outcome as a
late command before); and the stale-ACKNACK case after a reader resets its
writer proxy, which needs a decision on what ad9cfde re-serves.
Validation (macOS)
cargo test -- --test-threads=1with and without--features security(695 / 705 lib tests, the integration tests, 58 doctests; one later security
run had
test::timestamppick up a sample from a robot sharing domain 0 onthe network, and passed on a private domain);
cargo clippy --all-targets(both feature sets) clean;
cargo +nightly fmt --check.Developed with assistance from Claude Fable 5.1 and OpenAI Codex (gpt-6-astra).