Fix/wait drain only ready fds - #57
Merged
Merged
Conversation
added 2 commits
August 11, 2026 17:29
… queued recv_from peeks first to size the buffer. A zero-length datagram makes that peek return 0 and the function bailed out without ever dequeuing it. MSG_PEEK does not consume, so the datagram stayed queued and the socket stayed readable forever, spinning every wait that polls that fd. Consume and drop it, matching how a runt datagram is already handled.
rmw_wait made three passes over every entity in the wait set on every call: drain every socket, re-arm every fd with epoll_ctl, then drain every socket again after epoll returned. For a wait set of 719 entities that is roughly 2150 syscalls per call, nearly all returning EAGAIN or EEXIST. epoll_wait already reports exactly which fds are ready, so use that: - The wait set remembers what each fd was armed for (UdsWaitSet::armed), keyed by fd and validated by a per-entity uid, so the arming pass costs no syscall in the steady state. The uid is needed because a fd number can be reused after its owner closed and the allocator may hand the new entity the same address, so (fd, pointer) cannot tell them apart. - Both full drain passes become one walk over ready_events. - Entities holding already-queued messages are still found by checking the internal queues, then confirmed with a single non-blocking epoll_wait so a message sitting in a socket is still reported in this call rather than the next one. A wake alone no longer ends the wait, only real progress does: an entity with data, or a guard condition that fired. A doorbell ring, or a datagram that drain_socket filters out (ignored local publication, foreign message type), leaves nothing ready and re-blocks for the caller's remaining time. Syscalls per call go from ~2150 to 1 + 2 per ready entity. Closes #51
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the wait/transport plumbing to avoid spinning on permanently-readable fds by (1) explicitly consuming zero-length datagrams in recv_from, and (2) making rmw_wait drain only epoll-reported ready fds using a persistent fd→entity arming map.
Changes:
- Add handling + test coverage to ensure zero-length datagrams are consumed (not left queued) to prevent busy-waiting.
- Introduce per-entity monotonic
uidand a per-wait-setarmedmap to track what each epoll fd is armed for across calls. - Refactor
rmw_waitto avoid unconditional full wait-set drains; instead, epoll waits and drains only ready fds and avoids syscalls in steady state.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rmw_unix_socket_cpp/test/test_transport.cpp | Adds a regression test verifying recv_from() consumes zero-length datagrams so the socket doesn’t stay readable forever. |
| rmw_unix_socket_cpp/src/types.hpp | Adds entity uids and a wait-set armed map to support stable fd arming + correct fd reuse detection. |
| rmw_unix_socket_cpp/src/transport.cpp | Ensures zero-length datagrams are dequeued (and warns) so they don’t cause wait loops to spin. |
| rmw_unix_socket_cpp/src/rmw_wait.cpp | Refactors wait logic to arm epoll fds once and drain only ready fds, aiming for O(ready) behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
326
to
333
| if (guard_conditions) { | ||
| for (size_t i = 0; i < guard_conditions->guard_condition_count; ++i) { | ||
| if (!guard_conditions->guard_conditions[i]) {continue;} | ||
| auto * gc = static_cast<rmw_uds::UdsGuardCondition *>( | ||
| guard_conditions->guard_conditions[i]); | ||
| register_fd(gc->eventfd_fd); | ||
| gc_index[gc] = i; | ||
| register_fd(gc->eventfd_fd, rmw_uds::ARMED_GUARD_CONDITION, gc, gc->uid); | ||
| } |
This was referenced Aug 13, 2026
benaliabderrahmane
added a commit
that referenced
this pull request
Aug 14, 2026
…sion - WaitDoesNotStealTriggerOfGuardConditionNotWaitedOn: a GC armed earlier but absent from this call's array must keep its trigger (fails before the armed-dispatch gate lands). - RecvFromSkipsZeroLengthDatagramToRealMessage: junk must not end a drain with a real message queued behind it (fails before the recv_from restructure). - RecycledFdNumberIsRearmedForNewEntity: pins the armed-cache uid check. - QueuedBacklogStillReportsSocketDataWithoutBlocking: pins the poll-only pass contract and its non-blocking property. - BoundedWaitUnderRegistryChurnTimesOutOnSchedule: pins the doorbell-only re-block loop under registry churn. - ResponseDeliveredThroughWaitOnClient: first coverage of the client arm of the wait dispatch. - GraphQueryThrottlesStaleCleanup: private domain 93 (the old domain-99 negative raced every parallel test binary's unthrottled rmw_init sweep), deterministic throttle stamps instead of wall-clock races, a sweep-resume assertion, and unconditional ghost cleanup.
benaliabderrahmane
added a commit
that referenced
this pull request
Aug 14, 2026
…poll ADDs to polling The armed-fd cache survives across rmw_wait calls, is insert-only, and fds were never EPOLL_CTL_DEL'd — so the epoll dispatch loop would act on any fd ever armed in the wait set, whether or not this call's arrays contain its entity. A guard condition owned by another wait set had its eventfd consumed with the trigger recorded nowhere (a permanent lost wakeup for the owner: wait_for_service and GraphListener hang), a subscription's socket could be drained by a wait set not waiting on it, and under fd duplication (fork without exec, dup, SCM_RIGHTS) a destroyed entity's stale entry was dispatched as a use-after-free. Gate dispatch on membership in armed_this_call — the fds this call's arming pass touched. A ready fd outside the set is EPOLL_CTL_DEL'd and erased without dereferencing its entity: the DEL keeps a readable level-triggered fd from spinning the loop, and the erase re-arms the entity on the next call that waits on it (this also stops the map growing unboundedly under entity churn). Steady-state cost: one hash insert per entity per call, zero syscalls. register_fd also no longer swallows non-EEXIST epoll_ctl failures: post-#57 there is no unconditional pre-drain, so a silently unarmed entity was permanently invisible under e.g. epoll watch exhaustion. Log the error, drain the entity directly each call, and bound the block at 200 ms so the drain recurs — devel's degraded-but-correct polling, for only the failed fds. Also drain past a full 64-event batch in the poll-only pass, and correct the UdsWaitSet::armed safety comment: close() alone does not guarantee epoll removal while another descriptor references the open file description.
Owner
Author
|
Full adversarial review completed (same process as #60: multi-lens review workflow + adversarial verification of every finding, ~200 raw findings deduplicated to a canonical list). 12 defects on this PR — 2 blockers, 6 should-fix, 4 nits — all now fixed on this branch, plus 1 pre-existing bug found along the way. Blockers (both fixed in
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
rmw_unix_socket_cpp/src/rmw_wait.cpp:29
- rmw_wait.cpp now uses std::unordered_map (gc_index) but does not include <unordered_map>, relying on transitive includes from types.hpp. This is brittle and can break compilation if includes change; include the header directly here.
#include <unordered_set>
- WaitDoesNotStealTriggerOfGuardConditionNotWaitedOn: a GC armed earlier but absent from this call's array must keep its trigger (fails until the armed-dispatch gate lands). - RecvFromSkipsZeroLengthDatagramToRealMessage: junk must not end a drain with a real message queued behind it (fails until the recv_from restructure lands). - RecycledFdNumberIsRearmedForNewEntity: pins the armed-cache uid check, previously untested. - QueuedBacklogStillReportsSocketDataWithoutBlocking: pins the poll-only pass contract and its non-blocking property. - BoundedWaitUnderRegistryChurnTimesOutOnSchedule: pins the doorbell-only re-block loop under registry churn. - ResponseDeliveredThroughWaitOnClient: first coverage of the client arm of the wait dispatch.
…poll ADDs to polling The armed-fd cache survives across rmw_wait calls, is insert-only, and fds were never EPOLL_CTL_DEL'd — so the epoll dispatch loop would act on any fd ever armed in the wait set, whether or not this call's arrays contain its entity. A guard condition owned by another wait set had its eventfd consumed with the trigger recorded nowhere (a permanent lost wakeup for the owner: wait_for_service and GraphListener hang), a subscription's socket could be drained by a wait set not waiting on it, and under fd duplication (fork without exec, dup, SCM_RIGHTS) a destroyed entity's stale entry was dispatched as a use-after-free. Gate dispatch on membership in armed_this_call — the fds this call's arming pass touched. A ready fd outside the set is EPOLL_CTL_DEL'd and erased without dereferencing its entity: the DEL keeps a readable level-triggered fd from spinning the loop, and the erase re-arms the entity on the next call that waits on it (this also stops the map growing unboundedly under entity churn). Steady-state cost: one hash insert per entity per call, zero syscalls. register_fd also no longer swallows non-EEXIST epoll_ctl failures: with the unconditional pre-drain gone, a silently unarmed entity was permanently invisible under e.g. epoll watch exhaustion. Log the error, drain the entity directly each call, and bound the block at 200 ms so the drain recurs — the old degraded-but-correct polling, for only the failed fds. Also drain past a full 64-event batch in the poll-only pass, and correct the UdsWaitSet::armed safety comment: close() alone does not guarantee epoll removal while another descriptor references the open file description.
The zero-length cleanup consumed with a blind 1-byte recv issued after a separate, non-atomic peek. recv_from has no per-fd serialization, and the same socket is drained concurrently by the wait-side and take-side paths under a MultiThreadedExecutor: if a racing drain consumed the zero-length datagram between the peek and the discard, the 1-byte recv dequeued whatever real message was now at the head and silently destroyed all but its first byte. Let the n == 0 case fall through to the full-buffer consuming recv, which dequeues exactly one datagram whatever it turns out to be, and loop on consumed junk (zero-length and runt) so a single recv_from call returns the next real message or a genuinely empty socket. Returning false on consumed junk ended every drain loop with deliverable messages still queued behind the junk, costing one wake per junk datagram and 'nothing taken' rmw_take results with data queued.
Pre-existing (predates this branch): the consuming recv omitted MSG_TRUNC, so when a concurrent drain dequeued the peeked datagram and a larger one stood at the head, the kernel silently truncated it to the buffer sized for the smaller peek. The runt check passed and the size-mismatch branch clamped payload_len and returned true — delivering a truncated payload as if it were valid. With MSG_TRUNC the return value is the true datagram size; anything larger than the buffer is dropped with a throttled warning instead of queued as a prefix. Kept as its own commit so it can be cherry-picked to devel independently of the recv_from restructure.
benaliabderrahmane
added a commit
that referenced
this pull request
Aug 19, 2026
…ady-fds Fix/wait drain only ready fds
benaliabderrahmane
added a commit
that referenced
this pull request
Aug 19, 2026
rmw_wait.cpp conflicted (#57's armed-fd cache vs this branch's pull-based replay); resolved with the reconciliation reviewed and tested on integration/tl-pull-launch-fixes: watermarks passed to both subscription drains, the 200 ms bounded block covering unwired graph waits and unarmed fallback entities, and the armed_this_call dispatch gate. rmw_graph.cpp's sweep comment picks up the clause about the registry_add retry path this branch introduces. The resulting tree is byte-identical to integration/tl-pull-launch-fixes @ 85022eb.
Merged
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.
No description provided.