fix(rmw_wait): event-driven wakeup via a registry doorbell; remove the 200 ms poll - #39
Closed
benaliabderrahmane wants to merge 4 commits into
Closed
fix(rmw_wait): event-driven wakeup via a registry doorbell; remove the 200 ms poll#39benaliabderrahmane wants to merge 4 commits into
benaliabderrahmane wants to merge 4 commits into
Conversation
…ll; remove the 200 ms poll Three defects fixed together, with scenario tests that failed before: - A wait set now carries its context (set at rmw_create_wait_set) instead of scavenging it from the first subscription/service/client, so the top-of-wait registry check also runs for guard-condition-only waits (rclcpp's GraphListener shape). Before, a latched publisher on an idle node whose executor waited on guard conditions alone never replayed to late joiners. - Per-node graph guard conditions are now actually triggered on graph changes: the context keeps a mutex-guarded list (registered in rmw_create_node, removed before destruction in rmw_destroy_node) and rmw_wait triggers them when the registry generation changes. The old ctx->graph_guard_condition branch was dead code (never assigned); without this, wait_for_service could hang even though the service was up. - The 200 ms poll bound is gone. Every context binds a doorbell socket (registered as ENTRY_DOORBELL before the first generation snapshot); every registry mutation rings all doorbells strictly AFTER the generation bump, and rmw_wait drains its doorbell strictly BEFORE reading the generation, so no wakeup can be lost. Doorbell-only wakes re-check the registry and re-block: RMW_RET_TIMEOUT surfaces only at the caller's own deadline (previously a 600 ms wait returned TIMEOUT at 200 ms), and infinite waits block until a real event. Slot teardown already unlinks socket files, so graceful exit and the stale-PID reaper both clean the doorbell up. Known limit: a process built before this change bumps the generation but never rings, so mixed old/new fleets should upgrade together (same as the shm payload flag). A latched publisher in a process that never calls rmw_wait still cannot replay; that pre-existing gap is unchanged. Full suite green on Jazzy (128 tests): replay now reaches a late joiner ~20 ms after it joins, and the graph guard condition fires ~1 ms after a graph change. Review hardening (adversarial panel findings, all reproduced before fixing): - ring_doorbells: AF_UNIX datagrams stay charged to the sender until the receiver consumes them, so one unresponsive participant could exhaust the ring socket's budget and silently mute wakeups to healthy peers — the ring fd is now recreated on EAGAIN and the send retried once (on a fresh fd, EAGAIN can only mean the destination's own queue is full, i.e. a wakeup is already pending). Regression test LatchedTopicSurvivesAnUnresponsiveParticipant fails on the old code and passes now. - ring_doorbells: the slot type is re-validated inside the seqlock window, so a slot recycled to a data endpoint mid-scan can no longer receive the wake octet. - The per-thread ring socket is closed at thread exit (RAII) instead of leaking one fd per mutating thread.
The rmw-level suites now assert only behavior observable through the public rmw API, so they would run near-verbatim against any RMW: - qos: large-message and shm tests keep the byte-equal delivery oracle and drop every shm_seg/ring/message_cache inspection; the EMSGSIZE trio injects the small send buffer through a new RMW_UDS_TEST_SNDBUF init seam (same pattern as RMW_UDS_TEST_FORCE_SHM_FAILURE) on a test-owned context instead of setsockopt on a private fd; the dead-peer soft-drop test forks a real subscriber process that exits uncleanly instead of unlinking a private socket path; the latched-depth test asserts the exact surviving values. - pub_sub: the corrupt-mid-batch take test reproduces the condition with a second same-topic publisher of a different type instead of hand-built wire datagrams; the large-message test keeps only the roundtrip oracle. - guard_condition: trigger semantics proven through the rmw_wait contract (one wait returns it ready, the next times out) instead of reading the eventfd. - service_client: shm-ring asserts dropped; wait-set no longer leaks on a failing assertion. - graph: gid comparison uses real publisher gids from rmw_get_gid_for_publisher instead of fabricated bytes. - The two replay-cache pruning tests whose oracle is a private field move to test_internal_publisher_cache.cpp, an explicit white-box suite. - registry unit tests (deliberate white-box): positive survivor assertions in the overflow test, the high-water assert relaxed to the scan invariant, and the two concurrency tests loop to an observation floor with a hard time cap so a starved CI runner can neither fail nor pass them vacuously. Full suite green on Jazzy (130 tests).
devel is now the integration branch (contributor PRs land there before main), so it needs the same CI coverage.
…wiring - Wait section: the wait set carries its context; the graph check runs for guard-condition-only waits; step 4 blocks with no internal poll and honors the caller's deadline. - New subsection: the doorbell — why an AF_UNIX datagram socket over the alternatives, the ring-after-bump / drain-before-check ordering pair that makes lost wakeups impossible, the best-effort edge cases (sender-budget recreate on EAGAIN, seqlock type re-validation, per-thread ring socket), crash cleanup via the existing reaper, and the two accepted limits. - Graph guard condition sections: the old 'wiring gap' text replaced by the per-context list of per-node guard conditions that is actually triggered. - Registration: the generation bump is followed by the doorbell ring. - Operational requirements: /tmp/ros2_uds must be exempted from tmp cleaners. - Limitations: notification requires a thread inside rmw_wait.
benaliabderrahmane
marked this pull request as draft
July 23, 2026 11:49
Owner
Author
|
Superseded by #42 and #43, which split this PR's two independent defects apart.
The split is not cosmetic: each half now carries a test that fails without it, and the rosbag2 end-to-end case was measured on all three builds — Splitting also surfaced a defect in this PR's own test: The test-decoupling commit ( Closing in favour of the two replacements. |
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
1 participant
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.
Problem
Three related defects around graph events and
rmw_wait:ctx == nullptr, so a latched publisher on an idle node never replayed to late joiners.ctx->graph_guard_condition(the only thing the check triggered) was never assigned anywhere — dead code.wait_for_servicecould hang even though the service was up.RMW_RET_TIMEOUTat 200 ms regardless of the caller's deadline (rclcpp::wait_for_message(-1)observably misbehaves), and every process woke 5×/s at idle.Fix
rmw_create_wait_set; the scavenging block is gone.rmw_create_node, removed before destruction), triggered on registry generation change.ENTRY_DOORBELLbefore the first generation snapshot); every registry mutation rings all doorbells strictly after the generation bump;rmw_waitdrains its doorbell strictly before reading the generation — that ordering pair makes lost wakeups impossible. Doorbell-only wakes re-check the registry and re-block;RMW_RET_TIMEOUTsurfaces only at the caller's own deadline; infinite waits block until a real event. No new threads, no registry layout change.Review hardening
An adversarial review pass found and fixed three defects in the initial doorbell, each reproduced first:
EAGAINand the send retried once (regression testLatchedTopicSurvivesAnUnresponsiveParticipantfails on the old code).Tests
All scenario-level, public-API only (
test_rmw_qos,test_rmw_wait): latched topic reaches a late joiner while the publisher's process idles; the node's graph guard condition fires on graph change; a 600 ms wait blocks the full 600 ms; latched delivery survives an unresponsive participant. Each failed before its fix. Full suite green on Jazzy — 130 tests — with replay reaching a late joiner ~20 ms after it joins and the graph GC firing ~1 ms after a graph change.The second commit decouples the rest of the rmw-level suites from implementation internals (behavioral oracles only; fault injection via env-var seams and a forked dead-peer process; deliberate white-box tests segregated into
test_internal_publisher_cache.cpp).Notes
rmw_waitstill cannot replay (pre-existing, unchanged).develto the CI triggers so this and future PRs todevelget checks.