Skip to content

Do not drop endpoint announcements or ACKNACKs on full channels - #450

Open
johannesschrimpf wants to merge 2 commits into
Atostek:masterfrom
johannesschrimpf:fix/no-silent-channel-drops
Open

johannesschrimpf wants to merge 2 commits into
Atostek:masterfrom
johannesschrimpf:fix/no-silent-channel-drops

Conversation

@johannesschrimpf

@johannesschrimpf johannesschrimpf commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Two internal channels of a DomainParticipant drop a message when they are
full:

  • The DP event loop tells Discovery about each new local reader and writer
    over Discovery's 64-slot command channel with try_send
    (src/rtps/dp_event_loop.rs:1316-1331 at c076919). When the channel is
    full, 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 announced
    and 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/writer uses it too) while Discovery
    is busy with other participants' SEDP samples or waiting for room in its
    own notification channel to the event loop.
  • The message receiver forwards ACKNACKs and NACK_FRAGs to the local writers
    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.

  1. Retry queue for discovery commands. A command that does not fit stays
    in a VecDeque on the event loop, later commands queue behind it (order
    kept), 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 of
    the security feature are pre-existing and left for a follow-up. A
    command is dropped only when Discovery has exited, which on shutdown is
    the normal order and is logged at info.
  2. Unbounded ACKNACK channel. Both ends of that channel are on the event
    loop thread, so back-pressure would deadlock at once; the channel becomes
    mio_extras::channel::channel() and forwarding uses send. What is
    queued 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 its
commit's non-test changes reverted:

  • every_new_local_endpoint_is_announced_to_discovery: an event loop with a
    one-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 AddLocalReader commands must arrive in order. Fails without
    commit 1 (only the first arrives), passes with it.
  • acknack_burst_reaches_writers: 200 ACKNACK datagrams from 200 readers
    before 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

AddTopic is still try_send under the DiscoveryDB lock (only the
DCPSTopic sample is lost; matching does not use it); the security feature's
blocking sends; endpoint removal's try_send_timeout on 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=1 with and without --features security
(695 / 705 lib tests, the integration tests, 58 doctests; one later security
run had test::timestamp pick up a sample from a robot sharing domain 0 on
the 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).

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant