Skip to content

refactor(drain): collapse the four socket drains into one - #65

Open
benaliabderrahmane wants to merge 3 commits into
develfrom
refactor/unify-socket-drain
Open

benaliabderrahmane wants to merge 3 commits into
develfrom
refactor/unify-socket-drain

Conversation

@benaliabderrahmane

Copy link
Copy Markdown
Owner

Description

The receive loop existed in four near-identical copies: drain_subscription
in rmw_subscription.cpp, drain_socket in rmw_wait.cpp, and an inline copy in
each of rmw_take_request and rmw_take_response. They had drifted, and the
drift 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. DrainTarget carries the
per-endpoint policy (queue and its mutex, depth cap, accepted msg_type, shm
reader cache, ignore_local_publications, the TRANSIENT_LOCAL watermark map, the
listener callback, the name to log) and the drain_target() overloads build it
from 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:

  • The rmw_wait drain now fires a subscription's on_new_message callback.
    Only the rmw_take path did, so a callback registered by an executor was
    silent for every message that arrived while a thread sat in rmw_wait.
  • The rmw_wait drain now reports queue overflow. It trimmed to the QoS depth
    silently, so messages dropped by a slow subscriber were only visible when the
    drop happened to land on a take.
  • Request and response queues are capped on the take path too. rmw_wait
    has 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 behind
    grew the queue without limit. That bound is now named SERVICE_QUEUE_DEPTH
    and 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, the
per-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 that
matters here: the unified drain has to preserve the replayed_watermarks dedup
and the depth trim. test_rmw_wait.cpp covers the lost-wakeup regressions in the
file 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_ns in four files, wall_now_ns in rmw_wait.cpp) were all the
same system_clock nanosecond reading under different names, so consolidating
them to one is a rename, not a semantic change.

Abderahmane BENALI and others added 3 commits September 18, 2026 14:36
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
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