feat(kio): add WaiterCell and Queue - #2560
Conversation
WaiterCell retains a strong Waiter across Context-based polls, so poll_* trait methods (and kio's own wait/Pending adapters) can drive waiter-based polls without losing their list registrations. It reuses the retained waiter after a wakeup when it would wake the same task and has no live registrations, avoiding an Arc allocation per poll; otherwise it replaces it so WaiterList GC can reclaim the retired slots. Deque is a poll-native FIFO queue in the Shared style: role-less clone-able handles, bounded or unbounded, split push/pop waiter lists, explicit close with drain-before-Closed pops. poll_push_with builds the item only once there is room, so a pending push costs nothing and needs nothing handed back. Both are for building poll-first protocol state machines on kio (qmux is the first consumer). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
WalkthroughThe PR introduces a poll-native FIFO 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
rs/kio/src/deque.rs (2)
262-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the inline test module
tests.Other kio modules use
mod tests; this one ismod test.♻️ Proposed change
#[cfg(all(test, not(loom)))] -mod test { +mod tests {As per coding guidelines: "Keep Rust tests inline as
#[cfg(test)] mod tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/kio/src/deque.rs` around lines 262 - 263, Rename the inline test module in deque.rs from test to tests, preserving its existing test configuration and contents. Ensure it follows the project convention of #[cfg(test)] mod tests.Source: Coding guidelines
150-159: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
pushsilently drops the item on close.Documented, but a
pushthat loses data on a race withcloseis easy to misuse; consider returningPushError<T>(or aClosed-with-item variant) so the caller can recover the item, matchingtry_push.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/kio/src/deque.rs` around lines 150 - 159, Update the `push` method to return a recoverable error such as `PushError<T>` when the queue closes before accepting the item, preserving the item for callers just as `try_push` does. Adjust the `crate::wait` closure and method documentation to propagate the item on closure rather than silently dropping it, while retaining the existing successful push behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/kio/src/deque.rs`:
- Around line 65-72: Replace all em dashes in the affected documentation and
comments: in rs/kio/src/deque.rs lines 65-72, change the Deque type
documentation dash to a semicolon; in rs/kio/src/deque.rs lines 124-130, change
the poll_push_with documentation dash to a comma clause; in rs/kio/src/waiter.rs
lines 168-176, replace both WaiterCell::register documentation dashes with
parentheses or commas; and in rs/kio/src/pollable.rs lines 40-42, change the
waiter field comment dash to a semicolon.
- Around line 9-15: Mark the public PushError enum as #[non_exhaustive] so
future variants can be added without breaking downstream exhaustive matches.
Leave its existing variants and documentation unchanged.
---
Nitpick comments:
In `@rs/kio/src/deque.rs`:
- Around line 262-263: Rename the inline test module in deque.rs from test to
tests, preserving its existing test configuration and contents. Ensure it
follows the project convention of #[cfg(test)] mod tests.
- Around line 150-159: Update the `push` method to return a recoverable error
such as `PushError<T>` when the queue closes before accepting the item,
preserving the item for callers just as `try_push` does. Adjust the
`crate::wait` closure and method documentation to propagate the item on closure
rather than silently dropping it, while retaining the existing successful push
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b4d19d6-3997-4e46-8d72-ab40515b8045
📒 Files selected for processing (6)
rs/kio/README.mdrs/kio/src/deque.rsrs/kio/src/lib.rsrs/kio/src/loom.rsrs/kio/src/pollable.rsrs/kio/src/waiter.rs
Prose rule bans em dashes; public error enums always get #[non_exhaustive] per the workspace error conventions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Future impl hand-rolled the same retain-across-polls dance WaiterCell now encapsulates, minus the reuse: it cloned the waker on every poll. The cell is taken out of the struct for the poll so its borrow doesn't overlap the &mut self it feeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The type is strictly FIFO (push back, pop front), so Deque named the VecDeque implementation rather than the role. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
register() collided with Waiter::register (registering with a list); the method hands back the waiter to use for this poll, so name it that. A Waiter clone now shares its identity: same shared Arc, so registrations made through either handle stay live until every clone drops. That is the escape hatch for a poll method that needs &mut self of the type holding the cell: clone the waiter to end the cell borrow. moq-net's Driver uses it in place of a mem::take dance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The method's job is keeping the waiter (and its registrations) alive until the next poll; name it after that. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/kio/src/queue.rs (1)
264-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
mod testtomod testsfor consistency.
waiter.rsusesmod tests(plural). As per coding guidelines: "Tests should be inline as#[cfg(test)] mod testsin the source file."♻️ Proposed rename
-mod test { +mod tests {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/kio/src/queue.rs` at line 264, Rename the inline test module declaration from mod test to mod tests in queue.rs, keeping its #[cfg(test)] annotation and test contents unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rs/kio/src/queue.rs`:
- Line 264: Rename the inline test module declaration from mod test to mod tests
in queue.rs, keeping its #[cfg(test)] annotation and test contents unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba1ae851-59ad-44e0-bded-121f1072b57b
📒 Files selected for processing (7)
rs/kio/README.mdrs/kio/src/lib.rsrs/kio/src/loom.rsrs/kio/src/pollable.rsrs/kio/src/queue.rsrs/kio/src/waiter.rsrs/moq-net/src/session.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/kio/src/pollable.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/kio/src/queue.rs (1)
133-144: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not invoke
makewhile holding the queue mutex.Line 143 executes caller-controlled code while
stateis locked. A closure can capture this queue and call another queue method, causing a deadlock; slow construction also blocks every queue operation. Preserve lazy construction by reserving capacity before unlocking and committing afterward, or redesign the API so user code does not run under the lock. Add a regression test for a reentrant factory with a timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/kio/src/queue.rs` around lines 133 - 144, The poll_push_with method currently invokes the caller-provided make closure while holding the queue state mutex, allowing reentrant deadlocks and blocking other operations. Reserve queue capacity and record the pending push under the lock, release it before calling make, then reacquire the lock to commit the item while preserving closed-state handling and capacity correctness; add a timeout-based regression test using a reentrant factory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rs/kio/src/queue.rs`:
- Around line 133-144: The poll_push_with method currently invokes the
caller-provided make closure while holding the queue state mutex, allowing
reentrant deadlocks and blocking other operations. Reserve queue capacity and
record the pending push under the lock, release it before calling make, then
reacquire the lock to commit the item while preserving closed-state handling and
capacity correctness; add a timeout-based regression test using a reentrant
factory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 917240d0-810b-4dc5-b404-943a76f56022
📒 Files selected for processing (4)
rs/kio/src/pollable.rsrs/kio/src/queue.rsrs/kio/src/waiter.rsrs/moq-net/src/session.rs
#353) * fix: release accept and ez waker registrations when a caller gives up The stream-accept paths kept pending accepters in a plain `Vec<Waker>` with no way to deregister. Entries were cleared only when a stream arrived or the accept failed, so a caller that started an accept and dropped the future — a `tokio::time::timeout` around `accept_uni()`, say — left its waker there for the life of the connection. The `will_wake` dedup only helped a task that re-polled; distinct tasks doing this on a quiet, long-lived connection grew the list without bound, retaining a task allocation each. `web-transport-quiche`'s `ez` layer had the same bug one level down, in more places: the driver's accept, datagram, handshake and open-capacity lists, and `ConnectionClosed`. That last one is the worst — every blocked read, write, accept and handshake parks on it, and it pushed a waker clone on *every* pending poll with no dedup at all, so it grew per-poll rather than per-task. Replace all of them with `kio::WaiterList`, whose entries are `Weak<Waker>` owned by the caller's `kio::Waiter`: a caller that walks away drops its waiter, the slot goes dead, and registration reclaims it. `ez`'s `poll_*` surface now takes `&kio::Waiter` instead of `&Waker` for that reason, and its async methods go through `kio::wait`. The "wake outside the lock" discipline is unchanged: `WaiterList::take` hands the list out of the lock exactly as the old `Vec<Waker>` returns did. Per-stream `blocked: Option<Waker>` slots stay plain wakers — a stream has a single reader and writer, so those have one owner and cannot accumulate. `SessionAccept` is shared by every clone of a session, so it also needs to fan one arrival out to every accepter parked on it, and the shared accept futures must not be polled with a caller's waker that can go stale. That is `AcceptWaiters`, in each backend's new `waiters.rs`, alongside `Parked` — the bridge a `Context`-based `poll_*` needs to keep its registration alive across polls. `Parked` mirrors `kio::WaiterCell`, which landed in kio after 0.5.2 (moq-dev/moq#2560). Once that releases, both copies can be deleted for it, along with the two-step make/park dance in the `AsyncRead`/`AsyncWrite` impls, which only exists because `Waiter` is not `Clone` in 0.5.2. Regression tests in every affected crate abandon accepters (and, for `ez`, reads and datagram reads) from distinct tasks and assert the wakers are released. Each fails against the unfixed code with all 256 retained. Note: `ez`'s public `poll_*` methods and `SessionAccept::poll_accept_*` now take `&kio::Waiter`, which is a breaking change; `kio` is re-exported from each crate root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: retire a parked waiter once its poll finishes The `Context`-to-`Waiter` bridge retained the waiter unconditionally, so a poll that returned `Ready` left the last poller's waker sitting in the cell until something polled it again or the stream dropped. A stream whose final read completed and then sat idle — or that was handed to another task — held that task's allocation for the rest of the connection. Bounded, unlike the list leak this branch fixes, but pointless retention all the same. Keep the waiter only while the poll is `Pending`: on `Ready` there is nothing left to wake, and dropping the waiter also releases whatever registration it made on the way (`error()` registers with the close list before a later step reports `Ready`). `Parked::poll` now takes the poll as a closure so the rule lives in one place and the call sites shrink to one line. The `AsyncRead`/`AsyncWrite` impls keep the two-step form — the closure cannot borrow `&mut self` while the cell is borrowed — but now hand the result to `park` so it applies the same rule. Reported by Codex on #353. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: wake accepters outside the locks they will take on resume `AcceptWaiters::wake_all` woke every parked accepter while holding its own mutex, and `SessionAccept::poll_accept_*` woke them while the caller held the lock on the accept state. A waker is free to resume its task inline, and the first thing a resumed accepter does is take both of those locks — so on such an executor a stream arrival deadlocks instead of being handed out. Under tokio the wake only schedules, which is why nothing has hit it. Both are now woken outside: `wake_all` takes the list under the lock and wakes the copy, and the wake moves out of `poll_accept_*` to a helper that runs it after the guard drops. Waking on `Ready` is equivalent to what the poll methods did — every internal `wake_all` immediately preceded a `Ready` return. The rest of the branch already had this discipline; `ez` returns its waiters out of the driver lock for exactly this reason. These two were the outliers. `waiters.rs` gains a unit test with a waker that re-enters the list from `wake`, which deadlocks against the previous commit (caught by a timeout, so a regression cannot hang CI). The first of these was reported by Codex on #353; the second is the same defect one level up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: document the accept poll contract, and pin the no-stacking invariant `SessionAccept::poll_accept_uni` / `poll_accept_bi` are public and now take a `&Waiter`, but carried only implementation comments — nothing in rustdoc said who owns the registration or how long it has to live. Say it: the registration is weak and owned by the caller, and a `Ready` is what the other accepters need waking for, which the caller does after releasing the lock. Also add a unit test for the invariant that makes the weak registrations work: each poll builds a fresh waiter and drops the previous one, so re-polling replaces its slot rather than adding one. That holds today through both routes (kio's `wait` and the `Context` bridge), and it is precisely what breaks if a future bridge reuses a still-registered waiter — the trap `kio::WaiterCell` documents. The test stacks 100 registrations if that regresses. Raised by CodeRabbit on #353. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix!: hold back accept wakes for the duration of a poll, and unlock before teardown wakes Two wake-under-lock paths survived the last pass, both of the same shape: a waker is free to resume its task inline, and the resumed task's first move is to take the lock the waker fired under. `poll_accept_*_shared` drives the shared accept futures with the accept list's own waker while holding the lock on the accept state, and an inner future can wake that list *inline* — `FuturesUnordered` notifies its parent from a child's `wake`, so a self-waking header decode is enough. `AcceptWaiters` now records a wake that arrives while a poll is armed and hands it back on `disarm`, which runs once the lock is gone. That closes the whole class rather than the one future that can reach it today. `on_conn_close` woke both connection-closed lists while still holding the driver guard — a named binding, unlike the temporaries everywhere else in that file, which is how it escaped the earlier sweep. It is also the worst place for it: a deadlock there strands the close and everything waiting on it. Reported by Codex. BREAKING CHANGE: `ez`'s `poll_*` methods and `SessionAccept::poll_accept_*` take `&kio::Waiter` instead of `&std::task::Waker`. A `Waker` cannot express deregistration — the caller has to own something whose drop releases the registration — so the leak these commits fix is not addressable without it. Callers move to `kio::wait` for the async path, or hold a waiter across polls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: keep accept wakes held until the last overlapping poll leaves `armed` was a flag, but a poll releases the accept lock *before* it disarms, so a second poll can be armed and running by then — and the first one leaving would clear the second's protection, exposing it to the inline wake the arming is there to prevent. Count the polls instead: a deferred wake stays owed until the last one leaves, and that one delivers it. Found while re-reading the previous commit, not by a reviewer; the flag version only misbehaves with two accepters overlapping on one direction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: cover the teardown wake, and correct two stale comments The teardown half of the wake-outside-the-lock fix had no test — reverting it failed nothing, while its sibling in `AcceptWaiters` was covered. Add one, with a waker that probes the driver lock from a scratch thread rather than taking it directly: a resumed task that simply blocks would wedge a runtime worker on a `std` mutex, and dropping the runtime waits for that worker, so the test would hang instead of failing. It now reports in about four seconds. Two comments went stale under their own commits: - quiche's cached-waker note still credited `ez` with `will_wake` dedup, which the `WaiterList` conversion removed. Replaced with the reason the other three backends already give. - the eight `poll_accept_*_shared` call sites still described `disarm` as clearing a flag, which stopped being true when `armed` became a count. Raised by review agents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: park before finishing, in the finished-poll tests Both tests delivered the payload before the tracked read began, so the first poll could return `Ready` outright. That still catches the regression — the waiter retained by *that* poll must be released — but it leaves the more interesting case to chance: a waiter retained from an earlier `Pending`, which the later `Ready` has to let go of. Send an opening byte so the stream can be accepted, drain it, then assert the tracked poll parks before the payload is sent. Which path the test takes is no longer up to timing. Raised by CodeRabbit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…on kio::Park (#358) * refactor(quiche): hand the keep-alive timer a waiter like the rest of ez The last poll method in `ez` still taking a `Context` was the keep-alive ticker's, so the driver built one from its waiter to call it. Take the waiter instead and build the `Context` at the one place that genuinely needs it, where tokio's `Interval::poll_tick` demands it. No behaviour change: the `Context` is made from the same waker, one level down. Everything else in `ez` already takes `&kio::Waiter`; the only `Context`s left are the `AsyncRead`/`AsyncWrite` impls, whose signatures are tokio's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor: build the poll bridge on kio::Park, now that it has shipped kio 0.5.3 released `Park` (the `WaiterCell` of moq-dev/moq#2560, renamed), so the hand-rolled retention in `Parked` goes away: it now wraps `Park` and inherits the reuse `Park` does — a steady-state cell allocates nothing, where this one built a fresh `Waiter` and `Arc` on every poll. What stays is the release-on-`Ready`. `Park` holds its waiter until the next poll or until it drops, which is right for a pending operation but not a finished one: the stream is done with that caller, and holding its waker pins the polling task's allocation until something polls the cell again. Neutering `settle` — that is, using bare `Park` — fails `a_finished_read_releases_the_poller`, so the wrapper earns its keep. 0.5.3 also made `Waiter` `Clone`, which retires the two-step dance in the `AsyncRead`/`AsyncWrite` impls. They took that shape only because the cell's borrow could not be ended before the `&mut self` poll; a cloned waiter shares the parked one's identity, so `hold` hands one out and the borrow ends there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: verify parked clones start idle --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Summary
WaiterCell: retains a strongWaiteracrossContext-based polls, bridgingpoll_*trait methods to kio's waiter-based polls.hold(cx)reuses the retained waiter after a wakeup (same task, no live registrations) to skip the per-pollArcallocation, and replaces it otherwise soWaiterListGC can reclaim retired slots.wait()andPendinguse it internally, picking up the same reuse.Waiteris nowClone, sharing identity: a clone wakes the same task and its registrations count as the original's, staying live until every clone drops. This is the escape hatch for a poll method that needs&mut selfof the type holding the cell: clone the waiter to end the cell borrow.Queue: a poll-native FIFO queue in theSharedstyle. Role-less clone-able handles, bounded or unbounded, split push/pop waiter lists so a push never wakes parked pushers, explicitclose()with drain-before-Closedpops.poll_push_withtakes a closure so a pending push builds nothing and hands nothing back.Drivernow retains its waiter withWaiterCellinstead of hand-rolling the same dance (and cloning the waker every poll).Motivation: qmux (moq-dev/web-transport) is being converted to a poll-first state machine implementing the new
web-transport-traitsans-I/O poll traits, built on kio. These are the two pieces kio was missing forContext-based poll surfaces;Queueshould also be reusable in moq-net (e.g. rebuildingTaskSet's submission channel withoutfutures::channel::mpsc).Public API changes
All additive, in
kio:pub struct WaiterCellwithnew(),hold(&mut self, &mut Context) -> &Waiter;Cloneyields an idle cell.impl Clone for Waiter(shared identity, see above).pub struct Queue<T>withnew(),bounded(usize),try_push,poll_push_with,push,try_pop,poll_pop,pop,close,is_closed,len,is_empty,capacity,same_channel.pub enum PushError<T>(Full/Closed,#[non_exhaustive]) withinto_inner().Test plan
cargo nextest run -p kio -p moq-net(648 tests, including WaiterCell replace/reuse/clone-identity and Queue unit tests).just loom: all 17 models (12 kio including 3 Queue wakeup/close races, 5 moq-net).cargo clippy --all-targets,cargo fmt,RUSTDOCFLAGS="-D warnings" cargo doc -p kioall clean.(written by Fable 5)