Do not warn when a status channel is full - #442
Merged
jhelovuo merged 1 commit intoSep 11, 2026
Conversation
`StatusChannelSender::try_send` logged
WARN StatusChannelSender cannot send new status changes, channel is full.
for a condition that its own doc comment ("Best-effort send. If there is no
receiver, this will fail silently.") and the very next line of its own body
("It is perfectly normal to fail due to full channel, because no-one is
required to be listening to these.") both declare to be normal.
It is not a rare condition. Every built-in Discovery endpoint
(DCPSParticipant, DCPSPublication, DCPSSubscription, DCPSParticipantMessage,
DCPSTopic) is an ordinary DataWriter/DataReader, so each gets a status channel
of capacity 4 from `sync_status_channel(4)` in pubsub.rs. Nothing ever drains
them: Discovery never calls `try_recv_status`, and the application has no
handle on these internal endpoints. Each discovered remote participant
produces one PublicationMatched/SubscriptionMatched event per built-in
endpoint, so from the 5th participant onwards every single match event is
dropped -- and warned about. The rate grows with the number of participants on
the domain and never stops, and the application cannot drain, suppress or act
on it.
Worse, `try_send` converted `Full` into `Ok(())` and dropped the payload. That
made the callers' own, deliberately quiet handling of `Full` unreachable dead
code -- `Writer::send_status` maps it to `()` with the comment "This is normal
in case there is no receiver", and `Reader::send_status_change` logs it at
`trace!`. Both were correct; neither could ever run.
Changes:
* `StatusChannelSender::try_send` now returns `Err(TrySendError::Full(t))`,
handing the unsent payload back, and logs nothing. The receiver is still
woken on a full channel, so a listener that has fallen behind is prompted to
drain. This revives the existing `Full` arms in `Writer::send_status` and
`Reader::send_status_change`.
* Added `StatusChannelSender::try_send_lossy` for the "dropping is fine" case,
logging a full channel at `trace!` and keeping `error!` for genuine send
failures. The six `DomainParticipantStatusEvent` senders use it; previously
they would have reported a full channel as `error!`.
Behaviour is otherwise unchanged: every status event that fits is still
delivered, in order, to whoever is listening.
Co-authored-by: Copilot & Claude Opus 5
Member
|
This seems to be an accidental regression in commit 724950c . Logging level was raised to find a bug, but it was not lowered back after the bug had been fixed. Merged as minimal version that only reverts the accident. |
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.
While making MWE for PR 441, came across this. So RustDDS seem to send warnings unnecessary if no one is listening best effort messages. This is ai-generated pull request made with Claude Opus 5. The PR below:
Do not warn when a status channel is full
Summary
StatusChannelSender::try_sendlogsfor a condition that the same function documents, twice, as normal. Because
RustDDS's own built-in Discovery endpoints never drain their status channels,
this fires once per discovered participant per built-in endpoint, forever, and
the application can neither drain nor suppress it.
The bug
src/dds/statusevents.rs:The doc comment promises to "fail silently". The comment on the line after the
warn!says the condition is "perfectly normal". The comment three lines latersays "we do not consider this to be an error". And yet it warns — at the one
level applications actually leave enabled in production.
Why it fires constantly
Every built-in Discovery endpoint —
DCPSParticipant,DCPSPublication,DCPSSubscription,DCPSParticipantMessage,DCPSTopic— is an ordinaryDataWriter/DataReader, so each gets a status channel of capacity 4(
sync_status_channel(4),src/dds/pubsub.rs:479and:1103).Nothing ever drains those four slots:
Discoverynever callstry_recv_status/as_status_evented/as_async_status_streamon its built-in endpoints (grep returns nothing).Meanwhile, each discovered remote participant produces one
PublicationMatched/SubscriptionMatchedevent on each built-in endpoint.So the channels fill after 4 participants, and from the 5th onwards every single
match event is dropped and warned about.
I confirmed this by instrumenting
Writer::send_status/Reader::send_status_change. On an ordinary office network, two bareDomainParticipants with no topics, writers or readers at all:— 18 distinct remote participants (other people's ROS 2 / Fast-DDS nodes) were
on that domain, and the events are genuine one-per-peer matches, not duplicates.
The warning rate is therefore proportional to how busy the domain is, and it
never stops.
The second half of the bug
try_sendconvertsFullintoOk(())and discards the payload. That makesthe callers' own, deliberately quiet handling of
Fullunreachable dead code:src/rtps/writer.rs:src/rtps/reader.rs:Both arms are correct. Neither can ever execute. The layer below already decided,
and decided the opposite way.
Why this is a bug worth fixing
WARNper event. One of them is wrong, and three separate comments in thefunction say the log is the wrong one.
number of participants on the domain. Applications cannot drain these
channels — they do not own the endpoints — and cannot lower the level without
turning off the whole
rustdds::dds::statuseventstarget, which would alsohide genuine problems.
WARNis what production leaves on. 578 linesper 10 s from the MWE below is enough to make the log useless.
went to the trouble of classifying
Fullas benign; the abstraction throwsthat away and loses the event payload with it.
MWE
Save the following code as new example in
examples/status_channel_spam_mwe/main.rsand run it to test.Run the MWE with command
With this PR the warnings dissappear.
The fix
StatusChannelSender::try_sendnow returnsErr(TrySendError::Full(t)),handing the unsent payload back, and logs nothing. The receiver is still woken
on a full channel, so a listener that has fallen behind is still prompted to
drain. This revives the existing
Fullarms inWriter::send_statusandReader::send_status_change, which already do the right thing. This is not abreaking change in practice, because StatusChannelSender is public-but-unobtainable.
If breaking change is ok, one could consider to set
StatusChannelSender::try_sendto be
pub(crate), as it's unobtainable anyway.Added
StatusChannelSender::try_send_lossyfor the "dropping is fine"case: it logs a full channel at
trace!and keepserror!for genuine sendfailures. The six
DomainParticipantStatusEventsenders(
dp_event_loop.rs×3,writer.rs,reader.rs,discovery_db.rs,discovery.rs) use it. Note these previously ended in.unwrap_or_else(|e| error!(...)), so withtry_sendmade honest they wouldotherwise have started reporting a full channel as an error — strictly
worse than the warning being removed.
Every status event that fits is still delivered, in order, to whoever is
listening. Nothing about matching, discovery or delivery changes.
Tests
Two new unit tests in
src/dds/statusevents.rs:full_channel_is_reported_to_caller_with_payload— a capacity-2 channelaccepts two events and then returns
Fullwith the unsent value, instead ofa silent
Ok(()).channel_accepts_again_after_drain— draining makes room again, and thereceiver sees exactly the accepted events in order; the rejected one leaves no
gap or duplicate.
cargo test→ 692 + 2 + 58 pass.cargo test --features security→ 702 + 2 + 58pass.
cargo +nightly fmtandcargo +nightly clippy --tests --examples(also
--all-features) are clean.Notes for reviewers
Discovery endpoints at all, since nothing can ever read them. That removes the
wasted work as well as the log line, but it is a much larger change to the
internal
pubsubAPI, and the logging contradiction above would still beworth fixing on its own.
is
try_send_lossy; raising itstrace!todebug!would be a one-linechange. I kept it at
trace!because the built-in endpoints hit this pathconstantly and by design.
reproducible.