Repository navigation
Conversation
Quest-Claim: def91ad129e6e8bf
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
moq-tokio's quic::driver, moq-uring's driver, and qmux delete their per-stream and per-connection waiter tables, Notifys, and stopped map, and wait on moq-quic's poll_* forms instead. They keep their I/O, timers, the driver kick, and the latch that publishes a local close once the close frame is out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every wait is a poll_* on &kio::Waiter, so the events that reported them go: StreamEvent, Event's HandshakeDataReady, Connected, HandshakeConfirmed, ConnectionLost, Stream, and DatagramReceived, and QMux's Event and poll. Event keeps PathUpdated. A reset that frees its stream keeps the acknowledged prefix, so poll_acked still answers for it once the Acked event's offset is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
poll_transmit takes the driver's waiter and returns Poll<Transmit>: with nothing to send it registers the waiter, which the application's writes, finishes, resets, stops, refused opens, returned credit, datagrams, new limits, and close wake. Pacing and loss recovery stay on poll_timeout. QMux's poll_transmit takes the same shape. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Handles no longer wake their driver after mutating the connection: moq-quic wakes the waiter the driver parked on poll_transmit. The drivers keep only their ingress wake. qmux's stalled writer waits on poll_closed, so a local close still starts its close bound while output is blocked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also tests the transmit wait, and formats. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also drops qmux's rustc-hash, which only its waiter maps used. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…c55ddeffe12 # Conflicts: # rs/moq-tokio/src/quic/driver/connection.rs
…ming it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts resolved against this branch's moq-quic-owned waits: - moq-tokio: handle drops take `lock_for_drop` (#7360), now in `StreamRef` too; the driver's poisoned-state `terminate` takes no shared argument, and the test poison helper no longer wakes a driver waker this branch deleted. - moq-uring: handles count toward the last-drop close (#7309); a dropped driver fails through `Inner::fail`, which also closes moq-quic so handles parked in its waits wake. - moq-quic tests: the congestion renames (#7258) and the keep-alive default (#7449). - quest poll.md keeps the follow-ups planned from this PR, minus the link to the waits quest this PR deletes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Settled in #7490: the latch is I/O behavior moq-quic can't see, publishing a local close only once the CONNECTION_CLOSE is staged (moq-uring) or delivered with the transport shut (qmux). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 50 minutes. View limit details
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. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: b423b27
No actionable bugs found in the changed areas reviewed. Centralizing waits in moq-quic removes duplicated waiter tables and event translation; retaining ingress wakes and transport-specific close-publication latches is justified by their distinct I/O responsibilities. I traced transmit wakeups, close/error propagation, receive draining, acknowledged-prefix retention after reset, and the lifetime of Tokio stopped() futures across the three adapters. The close-wait registration addresses the earlier local-review issue described in the PR.
Verification limits: static review of the diff and relevant surrounding code through GitHub; I did not run tests or benchmarks or independently reproduce the reported performance results.
(Written by gpt-6.1-sol)
There was a problem hiding this comment.
ℹ️ Minor suggestions only.
Reviewed changes
- Consumers wait through moq-quic's
poll_*— moq-tokio's driver, moq-uring's driver, and qmux drop their own per-stream/per-connection waiter tables, accept/open/datagram lists, and event sweeps; moq-tokio holds akio::Park, and itsstopped()futures keep the stream's ID until the last of the stream and its futures drops. poll_transmitis the driver's wait — both QUIC and QMux register the driver's waiter when they have nothing to send, and every application-side queue wakes them throughWaits::wake_transmit; pacing and loss recovery stay onpoll_timeout.- New waits, and deletions —
Connection::poll_handshake_data,Connection::error, andrecv::Stream::poll_received_resetreplace the last event-shaped waits;StreamEvent, the waitingEventvariants, andmux::Event/mux::Connection::pollare gone, leavingEvent::PathUpdated. endskeeps a reset's acknowledged prefix —poll_ackedstill answers for that prefix after the stream's state is freed.- Tests and benches — event-expectation tests became wake-counting tests, and a moq-tokio
fanoutbench swept over streams per connection is wired into Nightly.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
`local_address_changed` queues a PING and any RETIRE_CONNECTION_ID, so it wakes `poll_transmit` like every other application call; the doc lists it, and says why a new probe target needs no wake. qmux's latch doc names the stalled close bound. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (b423b2740):
local_address_changedwakes the parked driver — it now callsself.streams.waits.wake_transmit()after queueing the PING and anyRETIRE_CONNECTION_ID, so a sans-IO embedder that called it re-polls instead of leaving the frames queued.poll_transmitdoc names the new wake and the probe-target case — "a local address change" joins the wake list, and a new probe target is documented to wait for the next poll, since a wake inset_probe_targetwould spin both drivers, which call it every poll.- Regression test —
waits::transmit_hears_a_local_address_changeparks the driver, callslocal_address_changed, and asserts the wake plus that the PING is whatpoll_transmitfinds. - qmux close-latch docs —
closed/local_closenow name the stalled path ("gave up after [CLOSE_TIMEOUT]"), matchingwriter_done/run_writer/poll_stalled.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
|
quest land: this pull request conflicts with a pull request queued ahead of it, in these files:
Once one of them lands, this one conflicts with the base and is ejected when its turn comes. Resolve the overlap before then, such as by coordinating with that author, or by merging the base once it lands. |
|
codex / Codex land fixer note:
(written by codex / Codex) |
|
quest land removed this pull request from the queue: this maintainer pick already used its mechanical fix attempt. Run /quest-iterate to diagnose the repeated failure, then pick it again. |

Problem
Finishes quest/m1/quic-api/waits.md. moq-dev/moq#7391 gave moq-quic its own waiter lists and
poll_*forms, but moq-tokio'squic::driver, moq-uring's driver, and qmux still kept their own per-stream and per-connection waiter tables, fed byStreamEventand the connection'sEvents. Every handle also kicked its driver after mutating the connection.Approach
Consumers wait on moq-quic. moq-tokio's
quic::driver, moq-uring's driver, and qmux now wait only through moq-quic's poll forms. They no longer have:readable/writable/finishingandblocked_readers/blocked_writersmaps;Notifys;stoppedmap and the event sweeps.moq-tokio's futures hold a
kio::Park. Itsstopped()futures keep the stream's ID until the last of the stream and its futures drops, so the end that moq-quic keeps untilrelease()still answers them.New waits in moq-quic, for the last two event-shaped waits.
Connection::poll_handshake_datareplacesHandshakeDataReady, used by moq-tokio'sConnecting::handshake_data.recv::Stream::poll_received_resetreplaces the readable wake behindreceived_reset. Also new:Connection::error(), which says why the connection was lost.Events deleted.
StreamEventis gone, and so are theEventvariants a handle waited on (HandshakeDataReady,Connected,HandshakeConfirmed,ConnectionLost,Stream,DatagramReceived). QMux'sEventandpollare gone too.StreamsState::notify's event-to-list mapping became direct wakes.EventkeepsPathUpdated, which no handle waits on.poll_transmitis the driver's wait. Both QUIC and QMux take the driver's waiter and returnPoll. With nothing to send, it registers the waiter. These wake it:set_limits;Pacing and loss recovery stay on
poll_timeout. Every handle-side kick is deleted. Each driver keeps only its ingress wake, so a received datagram or record still re-arms timers.qmux close while output is blocked. qmux's stalled writer also registers on
poll_closed, so a local close still starts the close bound while output is blocked.a_close_behind_a_stalled_peer_is_boundedcovers this on paused time. It fails (3.9 s, over the 3 s bound) without the registration.Tests count wakes. moq-quic's
Eventtests now count wakes with a counting waiter. New tests cover the handshake-data, received-reset, and transmit waits.Acked prefix kept after a reset. A reset that frees its stream keeps the acknowledged prefix alongside its end.
poll_ackedstill answers for that prefix after theAckedevent's offset is gone.Impact
StreamEventis gone, and so areEvent::{HandshakeDataReady, Connected, HandshakeConfirmed, ConnectionLost, Stream, DatagramReceived},mux::Event, andmux::Connection::poll.Connection::poll_handshake_data,Connection::error, andrecv::Stream::poll_received_reset.Connection::poll_transmit(waiter, now, max_datagrams, buf) -> Poll<Transmit>andmux::Connection::poll_transmit(waiter, now) -> Poll<Bytes>.rs/moq-quic/MIGRATE.md(a table from each event to its wait), and the README's fork list is updated.open_uni/open_bi/accept_uni/accept_bibecameasync fn, with the same signatures for callers.poll_open_requestafter a local close returns the close's error rather thanOk(None)(no credit).Decisions
closedwith oneWaiterList. They publish a close this side asked for topoll_closedonly once the CONNECTION_CLOSE is staged (uring), or delivered with the transport shut (qmux). That is I/O behavior moq-quic can't see, and keeping it changes no behavior. The reason is now documented next to each latch (rs/moq-uring/src/quic/driver/connection.rsState::closed,rs/qmux/src/session.rsState::closed). The alternatives were for moq-quic'spoll_closedto wait for the close packet to leavepoll_transmit(a regression for qmux), or to resolve atclose()and drop the guarantee.Choices made without asking
close()so every parked waiter wakes. moq-quic gained no "fail with a foreign error" API.poll_transmit's wait does not cover. moq-tokio's ingress already arrives on its own channel, so its driver waker is deleted.ShouldTransmitis still returned, but consumers ignore it.Chunks::finalizeand the lease methods still return it. moq-quic now wakes the transmit waiter itself.rust-extragroup, assh/gh/nightly.test.pyrequires of every bench target. qmux dropsrustc-hash, which only its waiter maps used.Measurements
Base is
origin/mainat 406ab9a, head is this branch. Each side ran once, sequentially, from separate target directories. A single run is noisy:schedule/*, which this branch does not touch, moved 0.70x to 1.64x.Driver fanout.
rs/moq-tokio/benches/fanout.rsis new and uses only the unchanged public driver API, so the same file ran on both sides. It opens N uni streams at once on one connection, writes 1 KiB on each, and reads them all, so up to N waiters are parked.No slope: the step from 256 to 1024 appears on both sides (1 MiB per iteration outgrows the window).
moq-quic/benches/streams.rs. The harness now drives reads with real waiters instead of events, so the end-to-end cases are not quite like for like. Nothing scales with stream count:connection_blockedis flat or better from 1 to 1024 live writers (0.79x to 1.06x).send_priorityat 1024 streams is unchanged (1.00x to 1.02x, plus one base outlier).set_priorityshowed 1.08x to 1.38x on the first run, but 0.96x to 0.97x on a rerun of that group alone.send_write_size/window_16k, at 1.13x to 1.25x. Those are flow-control-blocked writes on a 16 KiB window, and the cost does not grow with streams.Follow-ups
ShouldTransmitfrom moq-quic's public API, and test moq-tokio's close while the socket is blocked.unreachable!s on "released ID" paths in moq-tokio and moq-uringstoppedrun under the connection lock. They are invariants today, like the existing one inpoll_write.moq-tokio::integration backend::quic_client_close_drains_finished_trackfailed once under the full parallel run (subscribe failed: Unroutable,rs/moq-tokio/tests/integration/backend.rs:902), then passed 8 of 8 isolated runs and a full rerun. It looks like a load-dependent race between the auth grants and the subscribe, unrelated to the waits, but it was not reproduced onmain.Review
Head 4718586: local review skipped under the simple-review rule (moq-dev/quest#364). It is a one-line wake in
Connection::local_address_changed, a regression test, and doc comments, and it touches no public API, wire format, security code, orunsafe. Online review findings on b423b27:local_address_changedqueues frames without waking the parked driver: fixed. It now callswake_transmit, thepoll_transmitdoc lists it, andwaits::transmit_hears_a_local_address_changecovers it. I tried a wake inset_probe_targetas well and dropped it. Both drivers call that on every poll, so the wake made moq-tokio'ssim::*tests spin into their timeout. The doc now says a new target waits for the next poll.poll_transmit": refuted in part.writer_donepublishes only after the close batch is flushed and the transport shut, so that wording would be wrong. The doc now names the stalled-close bound (CLOSE_TIMEOUT) it left out. The latch stays, per the maintainer's 2026-11-01 decision to keep it (option (a)).Local checks on 4718586:
cargo nextest run --all-targets --all-features -p moq-quic -p moq-tokio -p moq-uring -p qmux@0.7.0(1543 passed), clippy-D warningson moq-quic and qmux, and rustfmt on the changed files.Previous local review, Codex, gpt-6.1-sol at medium effort, via
quest review --base origin/main. Latest round: a fresh session (round 1) on head 803ded3, with no findings. It covered theorigin/mainmerge, whose conflicts were resolved by hand. The current head b423b27 adds only a conflict-free merge oforigin/main(a quest deletion, moq-dev/moq#7496).What the merge resolved:
lock_for_drop(moq-dev/moq#7360), now inStreamReftoo. The poisoned-driverterminatetakes no shared argument here, and the testpoisonhelper no longer wakes the driver waker this PR deleted (the driver reaches the poisoned lock on its first poll or its timer).Drivernow fails throughInner::fail, which also closes moq-quic, so handles parked in moq-quic's waits wake rather than hang.Earlier rounds, on the pre-merge branch (session 1): round 1 (head 03ccfb4) had one finding, fixed in e1a5ef1: moq-tokio's driver also parks on the non-consuming
poll_closed, so a local close re-arms the closing timer while the socket holds output back. Rounds 2 (e1a5ef1) and 3 (4d71425) found nothing.Local checks on 803ded3:
just rs check --workspace(clippy-D warnings, fmt, docs, shear, sort).just rs test -p moq-quic -p moq-tokio -p moq-uring -p qmux@0.7.0: 1485 passed (see the flake under Follow-ups).just rs uring-check.cargo check --all-targets --all-featureson those four crates, andquest check --base origin/main.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code