Repository navigation
refactor(quic)!: expose driver polls and remove transmit hints - #7707
Conversation
Quest-Claim: e8ee455d33443ec8
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
kixelated
changed the title
|
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 10 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. CodeRabbit Verify
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 4ee591b
Exposing the existing waiter-based operations while retaining the internal scheduling decisions is a coherent simplification. No actionable bugs found in this static review. I did not run tests or exercise socket backpressure and concurrency stress.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ One minor cleanup: an unused dependency left behind. Nothing behavioral.
Reviewed changes
- Driver polls exposed under
&self, &kio::Waiter: both drivers'Connection(open/accept uni+bi, datagram receive, closed),Endpoint::poll_accept, andsend::Stream/send::Finished::poll_closedare public; the async forms wrap them, andsend::Finishednow implementsFuturewith the same outcome asstopped(). - Tokio endpoint waits: the three
tokio::sync::Notifyfields become caller-ownedkio::WaiterLists inside the state mutex,ref_countmoves ontoEndpointInner, and theAcceptfuture is deleted. ShouldTransmitremoved:recv::Stream::{lease, poll_lease, restore, release}andChunks::finalizedrop the transmit hint (returningResult/()), with each receive path waking the transmit waiter itself; callers in qmux, benches, fuzz, and tests are updated.- Uring WebTransport:
Session/SendStreamgain akio::Park, the handshake moves frompoll_fntokio::wait, and the callers forward to the now-public polls. - Tests and docs: a blocked-socket close test, a finished poll/future parity test, a finalize-wake assertion in the flow-control test, plus
MIGRATE.mdand quest updates.
I traced the two highest-risk surfaces myself and confirmed them with independent specialists: the endpoint Notify→WaiterList port wakes every parked waiter on the same guards as before (RecvState::handle/poll_forwards queue incoming only inside drive_recv, and EndpointDriver::poll wakes in that same locked section), and Finished's Future/poll_closed both route through StreamRef::poll_stopped on the single Wait::End list with a park disjoint from the polled state.
ℹ️ pin-project-lite is now an unused dependency
This PR removed the only pin_project! use in rs/moq-tokio — the Accept future in quic/driver/endpoint.rs — but rs/moq-tokio/Cargo.toml:89 still declares pin-project-lite and the driver feature still enables it at line 22. cargo shear reports unused optional dependency \pin-project-lite`` (a warning, so CI still passes).
Technical details
# Unused optional dependency `pin-project-lite`
## Affected sites
- `rs/moq-tokio/Cargo.toml:89` — `pin-project-lite = { version = "0.2", optional = true }`
- `rs/moq-tokio/Cargo.toml:22` — `driver = [..., "dep:pin-project-lite", ...]`
## Required outcome
- No `rs/moq-tokio` source references `pin_project`/`pin-project-lite` (`cargo shear` confirms `unused_optional_dependency`). Remove the dependency line and its `dep:pin-project-lite` entry from the `driver` feature, as AGENTS.md asks dependencies to earn their cost.
## Suggested approach (optional)
- `cargo shear --fix` (or the manual two-line edit), then re-run `just rs lint`.deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Keep both moq-tokio MIGRATE entries, and call Chunks::finalize directly in main's new storage and realtime tests now that it returns (). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Accept future was the last pin_project! user. Link the new MIGRATE headings to the PR like their neighbors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
recv::Stream::poll_received_reset takes a Context only for the WebTransport adapter, so a driver-only build flagged it as dead code. Gate it on quic like set_drop_stop beside it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Fixed in 26572823c1: removed (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head f2dd9c7 against base 0eff070.
No new actionable issue found in the base-aware update. The driver-only build fix gates the Context-based reset adapter on quic, the feature that includes the WebTransport adapter; the public waiter-based driver operations remain available with driver alone.
Static GitHub-only review; I did not build the feature combinations or run socket and concurrency tests.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta since the prior pullfrog review (4ee591b7b): the origin/main merge plus two follow-up commits.
- Merged
origin/main(0eff070d6), resolving the stream-storage test conflicts to callchunks.finalize()directly, adapting the newtests/realtime.rscongestion test the same way, and keeping both the #7707 and #7345MIGRATE.mdentries. - Removed the now-unused
pin-project-litedependency fromrs/moq-tokioand itsdriverfeature, with the matchingCargo.lockentry, and linked the new MIGRATE headings to this PR. - Gated
recv::Stream::poll_received_resetonquic, mirroringset_drop_stop, so thedriver-only build compiles without dead code.
The prior finding (pin-project-lite unused) is fixed. I confirmed no stale ShouldTransmit, let _ = chunks.finalize(), or old-signature lease/restore/release calls survived the merge.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Problem
The merged moq-quic waits own wakeups, but both drivers still hide their waiter-based polls. Callers also discard receive operations' obsolete
ShouldTransmithints, and Tokio's close wakeup under socket backpressure lacks controlled coverage.Approach
&self, &kio::Waiter; retain existing async methods as wrappers and implementFutureforsend::Finished.Notifywaits with caller-owned waiter registrations. Preserve incoming-attempt acceptance, buffered stream/datagram draining, and Uring's staged-close latch.ShouldTransmit, retain frame scheduling decisions as internal booleans, and update drivers, qmux, tests, fuzzing, and benchmarks.poll_closedregistration is removed, with idle timeout disabled and ingress suppressed after the stall. Test finished poll/future outcomes and use the future in Uring's receive-credit coverage.pin-project-litedependency; the deletedAcceptfuture was its last user.recv::Stream::poll_received_resetonquic, so the driver-only build (--no-default-features --features driver,aws-lc-rs) compiles again. That break predates this PR onmain; nightly's feature job hides it behind its earliercoroutinesfailure.Impact
leasereturnsResult<Lease, Error>andpoll_leasereturnsPoll<Result<Lease, Error>>; remove transmit-hint destructuring.restore,release, andChunks::finalizereturn(); call them directly.ShouldTransmitis removed.send::Finishedcan be awaited with the same outcome asstopped(). Existing async signatures and close/drain behavior are unchanged.Alternatives
Keep the approved caller-owned waiter shape and the merged moq-quic wait ownership. Retaining transmit hints or adding driver waiter tables would duplicate responsibility. Native WebTransport's adapter removal remains its own quest; this PR adapts its current Uring callers to the public polls.
Follow-ups
None discovered outside the existing native WebTransport and acknowledgment-hook quests. No maintainer decisions remain.
Validation
On f2dd9c7524, after merging
origin/mainat 0eff070d67:quest check --base origin/maincargo clippy --all-targets -D warningson moq-quic, moq-tokio, moq-uring, qmux, moq-bench and moq-quic-fuzz, and on moq-quic withfuzz,mux,sim,qlog.just rs test -p moq-quic -p moq-tokio -p moq-uring -p qmux@0.7.0: 1,525 passed, 38 skipped.sh/rs/tokio-features.sh --all,cargo doc -D warningson the touched crates,cargo fmt --check,cargo shear,cargo sort,cargo hakari generate --diff.let _ = chunks.finalize()calls, which now callfinalize()directly. The iroh removal (#7345) touches no driver wait.nix develop --command just ci(6,535 Rust tests passed) and the controlled mutation: removing Tokio's driverpoll_closedregistration fails the blocked-socket close test.Review
Codex,
gpt-6.1-sol, medium effort, round 1 reviewed f2dd9c7524 againstorigin/mainwith no findings.Posted reviews on 4ee591b7be:
pin-project-liteunused in moq-tokio. Fixed: removed fromCargo.toml, thedriverfeature, andCargo.lock.Originally written by GPT-6.1 Sol.
(Written by Claude Opus 5.5)