refactor(drain): collapse the four socket drains into one - #65
Open
benaliabderrahmane wants to merge 3 commits into
Open
benaliabderrahmane wants to merge 3 commits into
benaliabderrahmane wants to merge 3 commits into
Conversation
The receive loop existed in four near-identical copies (drain_subscription, drain_socket in rmw_wait.cpp, and inline in rmw_take_request and rmw_take_response) that had drifted: each decided for itself whether to cap the queue, warn about an overflow, or notify a listener. drain_endpoint() is now the only receive loop. DrainTarget carries the per-endpoint policy (queue and its mutex, depth cap, accepted msg_type, shm reader cache, ignore_local, the TRANSIENT_LOCAL watermark map, the listener callback, the name to log) and the drain_target() overloads build it from each impl struct, so a call site can no longer pick its own policy by accident. Where the copies disagreed the stricter one wins: the rmw_wait drain now fires on_new_message and reports overflow, and the request/response queues are capped at SERVICE_QUEUE_DEPTH on the take path too - recorded in drain.hpp as a deliberate tradeoff. Squashed from: - refactor(drain): collapse the four socket drains into one - docs(drain): record the take-path queue cap as a deliberate tradeoff - fix(drain): restore the <cstring> include, retarget the names this PR renamed
drain_endpoint() resolved the shm descriptor and then asked whether the
datagram should be ignored. For a subscription with
ignore_local_publications set, a large same-context publication was
therefore mapped out of the sender's ring and copied into the payload
buffer before being thrown away - up to SHM ring-record size of pointless
copying per message, on a path whose whole purpose is to avoid the copy.
is_same_context() reads only WireHeader::gid, which is on the wire before
anything is resolved, so the check moves ahead of the resolve. That is
the same reasoning the TRANSIENT_LOCAL dedup above it already gives for
its own position ("Checked before the descriptor resolve so a duplicate
never maps a segment").
Behaviour is identical either way: a resolve failure and an ignore both
end in `continue`. So the new test does not fail beforehand - it covers a
branch nothing reached. The two existing ignore_local tests publish small
inline payloads, which carry no descriptor, so the shm path combined with
ignore_local had no coverage at all.
Full suite green on Jazzy: 157 tests, 0 failures.
The headline behaviour change of the unified drain - rmw_wait now fires on_new_message, where only the rmw_take path did - had no test on this branch. WaitDrainFiresOnNewMessageCallback publishes three messages, waits without ever taking, and expects the callback to have been credited three events. It fails on devel (0 events) and passes here. The total is summed and awaited so the test stays valid once a listener thread delivers it. Also corrects the SERVICE_QUEUE_DEPTH comment: services and clients do carry a QoS depth, it is just not enforced on these queues. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUNQNo26cKRPrVXcaHZnje
benaliabderrahmane
force-pushed
the
refactor/unify-socket-drain
branch
from
September 18, 2026 12:44
23ad388 to
6e0554f
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
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.
Description
The receive loop existed in four near-identical copies:
drain_subscriptionin
rmw_subscription.cpp,drain_socketinrmw_wait.cpp, and an inline copy ineach of
rmw_take_requestandrmw_take_response. They had drifted, and thedrift is where the behavior gaps live — each copy decided for itself whether to
cap the queue, warn about an overflow, or notify a listener, so the same message
was treated differently depending on which path delivered it.
drain_endpoint()is now the only receive loop.DrainTargetcarries theper-endpoint policy (queue and its mutex, depth cap, accepted
msg_type, shmreader cache,
ignore_local_publications, the TRANSIENT_LOCAL watermark map, thelistener callback, the name to log) and the
drain_target()overloads build itfrom each endpoint's impl struct, so a call site can no longer pick its own
policy by accident.
This is the riskiest change in the series — it rewrites the hot receive path.
It is deliberately a standalone PR, with no feature on top, so the diff can be
read against the four originals.
Is this user-facing behavior change?
Yes — three differences that could not be preserved once the four copies became
one. All three are the copies disagreeing, resolved in favour of the stricter one:
rmw_waitdrain now fires a subscription'son_new_messagecallback.Only the
rmw_takepath did, so a callback registered by an executor wassilent for every message that arrived while a thread sat in
rmw_wait.rmw_waitdrain now reports queue overflow. It trimmed to the QoS depthsilently, so messages dropped by a slow subscriber were only visible when the
drop happened to land on a take.
rmw_waithas always trimmed both to 100; the take-path copies had no cap at all, so
a caller driving a service with
rmw_take_request()alone and falling behindgrew the queue without limit. That bound is now named
SERVICE_QUEUE_DEPTHand applied wherever the queue is filled.
Everything else is preserved exactly: check order (
msg_type, replay dedup,descriptor resolve,
ignore_local), the QoS depth cap on subscriptions, theper-message callback count of 1, and the receive timestamp.
How was this tested?
No new tests — the existing suite is the safety net for a refactor, and
test_rmw_qos.cpp(1572 lines, 20 TRANSIENT_LOCAL tests) is the one thatmatters here: the unified drain has to preserve the
replayed_watermarksdedupand the depth trim.
test_rmw_wait.cppcovers the lost-wakeup regressions in thefile this touches.
Clean rebuild (not incremental) plus full suite on jazzy: 156 tests, 0
failures. kilted, rolling and lyrical are on CI.
Did you use Generative AI?
Additional Information
Independent of #1 in this series; PRs 3-6 build on this one, so it wants to land
first among those.
Incidental finding while merging the copies: the five clock helpers involved
(
system_now_nsin four files,wall_now_nsinrmw_wait.cpp) were all thesame
system_clocknanosecond reading under different names, so consolidatingthem to one is a rename, not a semantic change.