Add a poll surface to web-transport-trait - #328
Conversation
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
5ae6a6d to
ae1bc2a
Compare
584956f to
b1cb991
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change introduces poll-based session, send-stream, and receive-stream interfaces with retained operation state. QMux stream reservations, reads, writes, terminal handling, and tests are adapted to polling. Iroh, Noq, Quiche, and Quinn integrations implement the new interfaces, while protocol headers use a fixed-size encoded representation. Examples and integration tests now use complete-buffer writes and mutable sessions where required. 🚥 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.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/qmux/src/session.rs`:
- Around line 2228-2248: In the pending-write resolution flow, add an early
return after calculating and releasing credit when `to_send` is zero, before
constructing or sending the `Stream` frame. Return `Poll::Ready(Ok(0))` so the
permit is dropped and no empty non-FIN frame is queued; preserve the existing
send and offset-update path for positive lengths.
- Around line 2504-2512: Update poll_closed to preserve existing self.buffer
contents before assigning newly polled stream.data; only replace the buffer when
it is empty, while retaining the current closed-state and polling behavior.
In `@rs/web-transport-trait/README.md`:
- Line 18: Correct the spelling of “underyling” to “underlying” in the README
sentence, leaving the surrounding wording and link unchanged.
In `@rs/web-transport-trait/src/lib.rs`:
- Around line 371-382: Update poll_read_chunk to explicitly limit the writable
destination to max bytes before calling poll_read_buf, rather than relying on
BytesMut::with_capacity(max.min(8 * 1024)). Preserve the existing 8 KiB
allocation cap and return behavior while ensuring the produced Bytes never
exceeds max.
🪄 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: 8cf104e6-bc5d-4be4-99ed-cccd7f54972a
📒 Files selected for processing (21)
rs/qmux/examples/h3qx.rsrs/qmux/src/session.rsrs/qmux/tests/backpressure.rsrs/qmux/tests/priority.rsrs/qmux/tests/qmux01.rsrs/qmux/tests/qmux02.rsrs/web-transport-iroh/src/recv.rsrs/web-transport-iroh/src/send.rsrs/web-transport-noq/src/recv.rsrs/web-transport-noq/src/send.rsrs/web-transport-quiche/src/ez/recv.rsrs/web-transport-quiche/src/ez/send.rsrs/web-transport-quiche/src/recv.rsrs/web-transport-quiche/src/send.rsrs/web-transport-quinn/src/recv.rsrs/web-transport-quinn/src/send.rsrs/web-transport-trait/README.mdrs/web-transport-trait/src/lib.rsrs/web-transport-trait/src/poll.rsrs/web-transport-trait/src/util.rsrs/web-transport-trait/tests/defaults.rs
…pter Streams get `poll_*` as the required methods, with the async ones as `poll_fn` wrappers. Every backend has a real `poll_read`/`poll_write` underneath, so nothing is boxed and nothing is implemented twice. Sessions stay async. No backend has a poll form of `accept_uni` or `open_bi` — they are multi-step routines over a shared connection — so a poll-native `Session` trait would be five copies of the same boxed-future adapter. `SessionPoll` provides that adapter once. It retains each in-progress operation, so a `Pending` poll resumes rather than restarting; that matters for `open_uni`/`open_bi`, which claim stream credit before they resolve. `poll_write_buf`/`poll_read_buf`/`poll_read_chunk` are overridable so the existing zero-copy paths survive, and quiche's `ez` poll methods are now public, letting its streams skip the adapter entirely. Two behavior fixes come along: - `write_chunk`'s default writes the whole chunk via `write_all_buf` instead of a single partial `write_buf`. - qmux's `write`/`write_buf` now do a partial write like every other backend rather than looping until the buffer drains. Callers that want the old behavior use `write_all`/`write_all_buf`. BREAKING CHANGE: implementors of `SendStream`/`RecvStream` must provide `poll_write`/`poll_read`/`poll_closed` instead of the async equivalents. Callers of the traits are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A write parked on flow control holds a slot in the session's outbound queue. The slot is not attached to a frame yet, so `outbound.remove` cannot find it and nothing else hands it back — dropping the retained future is the only thing that releases it. `reset`, `finish`, and a peer STOP_SENDING observed by `poll_closed` all left it behind, and the closed/fin guards at the top of `poll_write_buf` meant no later write would clear it either. The slot stayed gone for as long as the `SendStream` lived, and the queue only has 8 of them: reset a few streams mid-write and every other stream on the connection blocks forever. Route all three through `abandon_write`, and cover each independently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `!` marker and BREAKING CHANGE footer on the trait commit are enough for release-plz to pick the major bump and propagate it to the backends. Also add a static assertion that `OpState` is `Send + Sync`, which is the whole reason it holds a `Mutex`: embedding a bare boxed future in a backend's public `SendStream` would silently strip `Sync` from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…retention `write_chunk` promises the whole chunk but was built on a single `write_buf`, so a partial write truncated the stream silently. That was fixed without a test; add one, along with coverage for the rest of the provided methods, against streams that move one byte per poll so partial transfers are the normal case rather than an edge case. Also state what a `poll_*` implementation may retain between calls. Keeping its own progress is expected; keeping anything about the caller's buffer, or holding it past a terminal transition, is what produced the qmux reservation leak. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…raits
`PollSendStream` and `PollRecvStream` hold the `poll_*` methods and the
synchronous ones (`finish`, `reset`, `stop`, `set_priority`), with no `Send`
bound. `SendStream` and `RecvStream` sit on top and are now entirely provided
methods, so a backend implements the poll trait and opts in with an empty
`impl SendStream for MyStream {}`.
The point is what the poll traits *don't* require. A transport whose streams
are pinned to one thread — a thread-per-core `io_uring` runtime — can
implement them and be driven from a poll loop, which a `MaybeSend` supertrait
forecloses. `Session` still requires `Send + Sync`, so that stack isn't fully
expressible yet; the streams are the half that can be freed now.
Splitting rather than moving the bound onto each async method matters for
consumers: generic code holding `S: RecvStream` still gets `MaybeSend` through
the supertrait, so no call site grows a bound. What does cost something is the
synchronous methods moving — a module calling `finish` or `set_priority` needs
the poll trait in scope, which is one added name in an existing `use`.
An extension trait with a blanket impl was the other option, but the async
`write_chunk`/`read_chunk` overrides that keep quinn, noq, iroh, and quiche
zero-copy cannot be expressed against one.
BREAKING CHANGE: implementors move their `poll_*` methods, `type Error`, and
the synchronous methods to `PollSendStream`/`PollRecvStream`, then add an empty
`impl SendStream`/`impl RecvStream`. Callers that use the synchronous methods
need the poll trait imported.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`BytesMut::with_capacity` promises only a lower bound, and the default `poll_read_buf` hands out the whole spare capacity — so an over-allocation would let `read_chunk(max)` return more than `max`, which the method documents it won't. Cap the slice with `Buf::limit` instead of trusting the allocation to be exact. Not reproducible with bytes 1.12.1, which exposes exactly the requested capacity for every size from 1 to 8200; the guarantee is what's missing, not the current behavior. The new test pins it either way. Also add a qmux test for a retry whose buffer has drained to empty, and fix an "underyling" typo in the README. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bd70d43 to
2cb7e00
Compare
`Session` operations become `poll_*` on `&mut self`, with the async ones as provided helpers, matching what the stream traits already do. `Clone` is no longer required: a session that cannot be duplicated is now expressible, and applications that want a shared handle opt into it rather than every implementation paying for it. `&mut self` is what makes retained state safe here. A `&self` poll method that keeps an in-flight future has to either share one slot between concurrent callers — where the second caller polls the first's future with its own waker, so the first hangs and the resolved stream goes to the wrong task — or clone the session on every call. Taking `&mut self` gives each handle one owner by construction, the same property that makes the stream traits work. Concurrency comes from cloning the concrete session, where each clone gets its own slots. `SessionPoll` is gone: the trait is the poll surface now. Accept forwards natively wherever a backend already drives it from a poll loop — quinn, quiche, noq and iroh all have a `SessionAccept` built on `FuturesUnordered`, and forwarding preserves its per-caller waker handling. Everything else has no poll form on any backend, so `SessionOps` in the trait crate holds the retained futures; each backend adds one field instead of six. Those futures cannot borrow the session, so each async body was split into an owned-args form with the `&self` method delegating — one implementation, two entry points. qmux instead clones a handle into the future, because its bodies touch too many fields to thread through as arguments and its clone is refcount bumps only. Cached stream headers move to `web_transport_proto::Header`, an inline `[u8; 16]`. Two varints is the most a header can be, so it is `Copy` and the open path no longer allocates — cloning a `Vec` per stream would have defeated the point of caching the encoded bytes. BREAKING CHANGE: `Session` operations take `&mut self` and `Clone` is no longer a supertrait. Callers hold their session mutably and clone it for concurrent operations; implementors provide `poll_*` instead of the async methods. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/qmux/src/session.rs (1)
1824-1840: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid holding the accept/datagram mutex inside retained futures.
poll_accept_uni,poll_accept_bi, andpoll_recv_datagramclone aSessionand retain the resulting future inself.ops; the async body holdsself.accept_*/recv_datagram.lock().awaitacross laterrecv().awaitpolls. If a caller abandons the retained future after acquiring the lock, anotherSessionclone that later calls the same accept/datagram method is blocked for the lifetime of that retained operation. These futures should not own the mutex guard; poll the underlying receiver separately from the retained future path.🤖 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/qmux/src/session.rs` around lines 1824 - 1840, Refactor accept_uni_inner, accept_bi_inner, and the corresponding datagram receive helper so their retained futures do not hold accept_* or recv_datagram mutex guards across recv().await. Poll or receive from the underlying channels outside the retained Session future path, ensuring abandoned poll_accept_uni, poll_accept_bi, and poll_recv_datagram operations release the mutex before another Session clone accesses the same receiver.
🧹 Nitpick comments (3)
rs/web-transport-quiche/src/connection.rs (1)
56-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
#[allow(unused)].
header_datagramis read insend_datagram(Line 289) andmax_datagram_size(Line 314), so the allow no longer suppresses anything and hides future dead-code signal.♻️ Proposed change
header_bi: Header, - #[allow(unused)] header_datagram: Header,🤖 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/web-transport-quiche/src/connection.rs` around lines 56 - 57, Remove the stale #[allow(unused)] attribute from the header_datagram field in the connection struct. Keep header_datagram unchanged, since it is actively used by send_datagram and max_datagram_size.rs/web-transport-trait/src/util.rs (2)
236-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
cleared_future_restartsdoesn't assert the restart.It only checks
!is_pending()afterclear(). Poll again with a countingmaketo prove a fresh future is built.💚 Proposed test
fn cleared_future_restarts() { + let starts = Arc::new(AtomicUsize::new(0)); let mut state = OpState::default(); let waker = waker(); let mut cx = Context::from_waker(&waker); - assert!(state.poll(&mut cx, pending::<()>).is_pending()); + for _ in 0..2 { + let starts = starts.clone(); + assert!(state + .poll(&mut cx, move || { + starts.fetch_add(1, Ordering::Relaxed); + pending::<()>() + }) + .is_pending()); + state.clear(); + } + + assert_eq!(starts.load(Ordering::Relaxed), 2); state.clear(); assert!(!state.is_pending()); }🤖 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/web-transport-trait/src/util.rs` around lines 236 - 245, Extend the cleared_future_restarts test to verify that clearing rebuilds the future: use a counting make closure with state.poll, call state.clear(), then poll again and assert the counter increases and the new future’s result is observed. Keep the existing !state.is_pending() assertion.
103-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
#[non_exhaustive]onSessionOps.All fields are public and the struct is only ever built via
Default. Marking it#[non_exhaustive]lets you add op slots later without a breaking change for out-of-tree backends that might construct it literally.As per coding guidelines: "Treat public Rust API design as a compatibility commitment".
♻️ Proposed change
+#[non_exhaustive] pub struct SessionOps<S, R, E> {🤖 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/web-transport-trait/src/util.rs` around lines 103 - 118, Mark the public SessionOps struct as #[non_exhaustive] so future operation slots can be added without breaking external implementations. Keep its existing public fields and Default-based construction 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.
Inline comments:
In `@rs/web-transport-trait/src/lib.rs`:
- Around line 385-401: Update poll_read_buf and the poll_read API to accept
bytes::buf::UninitSlice directly, removing the transmute from UninitSlice to
[u8] and preserving the existing read-size and advance_mut behavior. Adjust all
implementations and call sites of poll_read to write into the uninitialized
destination without reading from it.
---
Outside diff comments:
In `@rs/qmux/src/session.rs`:
- Around line 1824-1840: Refactor accept_uni_inner, accept_bi_inner, and the
corresponding datagram receive helper so their retained futures do not hold
accept_* or recv_datagram mutex guards across recv().await. Poll or receive from
the underlying channels outside the retained Session future path, ensuring
abandoned poll_accept_uni, poll_accept_bi, and poll_recv_datagram operations
release the mutex before another Session clone accesses the same receiver.
---
Nitpick comments:
In `@rs/web-transport-quiche/src/connection.rs`:
- Around line 56-57: Remove the stale #[allow(unused)] attribute from the
header_datagram field in the connection struct. Keep header_datagram unchanged,
since it is actively used by send_datagram and max_datagram_size.
In `@rs/web-transport-trait/src/util.rs`:
- Around line 236-245: Extend the cleared_future_restarts test to verify that
clearing rebuilds the future: use a counting make closure with state.poll, call
state.clear(), then poll again and assert the counter increases and the new
future’s result is observed. Keep the existing !state.is_pending() assertion.
- Around line 103-118: Mark the public SessionOps struct as #[non_exhaustive] so
future operation slots can be added without breaking external implementations.
Keep its existing public fields and Default-based construction unchanged.
🪄 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: 3e68ef2e-2d37-4599-8c3a-a215939f32bd
📒 Files selected for processing (29)
rs/qmux/examples/h3qx.rsrs/qmux/examples/interop-client.rsrs/qmux/examples/interop-server.rsrs/qmux/src/session.rsrs/qmux/tests/backpressure.rsrs/qmux/tests/datagram.rsrs/qmux/tests/priority.rsrs/qmux/tests/qmux01.rsrs/qmux/tests/qmux02.rsrs/web-transport-iroh/src/recv.rsrs/web-transport-iroh/src/send.rsrs/web-transport-iroh/src/session.rsrs/web-transport-noq/src/recv.rsrs/web-transport-noq/src/send.rsrs/web-transport-noq/src/session.rsrs/web-transport-proto/src/header.rsrs/web-transport-proto/src/lib.rsrs/web-transport-quiche/src/connection.rsrs/web-transport-quiche/src/ez/recv.rsrs/web-transport-quiche/src/ez/send.rsrs/web-transport-quiche/src/recv.rsrs/web-transport-quiche/src/send.rsrs/web-transport-quinn/src/recv.rsrs/web-transport-quinn/src/send.rsrs/web-transport-quinn/src/session.rsrs/web-transport-trait/README.mdrs/web-transport-trait/src/lib.rsrs/web-transport-trait/src/util.rsrs/web-transport-trait/tests/defaults.rs
🚧 Files skipped from review as they are similar to previous changes (19)
- rs/qmux/examples/interop-client.rs
- rs/qmux/tests/qmux02.rs
- rs/qmux/examples/h3qx.rs
- rs/qmux/tests/priority.rs
- rs/web-transport-quiche/src/ez/recv.rs
- rs/web-transport-quiche/src/ez/send.rs
- rs/web-transport-trait/README.md
- rs/qmux/tests/qmux01.rs
- rs/qmux/examples/interop-server.rs
- rs/web-transport-trait/tests/defaults.rs
- rs/web-transport-quinn/src/recv.rs
- rs/web-transport-quiche/src/recv.rs
- rs/web-transport-iroh/src/recv.rs
- rs/web-transport-quinn/src/send.rs
- rs/qmux/tests/backpressure.rs
- rs/web-transport-noq/src/recv.rs
- rs/web-transport-iroh/src/send.rs
- rs/web-transport-quiche/src/send.rs
- rs/web-transport-noq/src/send.rs
Two problems the retained-future design surfaced. qmux's accept and datagram receivers were shared behind a `tokio::sync::Mutex`, and the retained future held the guard across `recv().await`. A handle that started an accept and stopped polling kept that guard, blocking every other clone until it was polled again or dropped — a hang, not a slowdown. It only became reachable once the future was retained rather than owned by the caller. Replace the shared receiver with `SharedRecv`, a `std::sync::Mutex` locked for the poll itself and a waker list so concurrent callers don't lose each other's registration — the same fix quinn's `SessionAccept` carries. accept and datagrams are now poll-native for qmux too, so three of its retained operations go away. The guard-across-await shape is no longer expressible: a `std::sync::Mutex` guard isn't `Send`, so the compiler rejects it. Separately, `poll_read_buf`'s default transmuted an `UninitSlice` into a `&mut [u8]`. Creating a reference to uninitialized memory is undefined behaviour whether or not the callee reads it. Zero the slice first — overridable to skip the memset. `poll_read_chunk` now reads into a `BytesMut::zeroed`, which drops its unsafe entirely and bounds the read to `max` structurally rather than via `Buf::limit`. The UB predates this PR — it came from `read_buf` on main — but it moved into a method this PR documents, so it is fixed here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Re the outside-diff finding on The diagnosis was right, including that it only became reachable once the future was retained rather than owned by the caller. A handle that started an accept and stopped polling kept the Rather than poll the receiver outside the retained path, qmux now shares it through The bad shape is also no longer expressible: a Covered by 🤖 Addressed by Claude Code |
The README still described `SessionPoll` and an async-native `Session`, both of which are gone. State what is actually there: everything is poll-native, `&mut self` is what makes retained operations safe, and concurrency comes from cloning the concrete session rather than a `Clone` bound on the trait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sessions now match the streams: `PollSession` holds the `poll_*` methods and the
synchronous ones, `Session: PollSession + MaybeSend + MaybeSync` adds the async
helpers as provided methods, and a backend opts in with an empty
`impl Session for MySession {}`.
Leaving them merged meant `Session` carried `MaybeSend + MaybeSync`, so a
thread-pinned session stayed inexpressible even after the stream traits were
freed — a thread-per-core stack could have `!Send` streams but no `!Send`
session to hang them off. `PollSession` carries no `Send` bound, and its
associated stream types only require the poll halves, so the whole stack is now
expressible. `a_not_send_session_can_implement_the_poll_surface` pins it.
As with the streams, the cost is that callers of the synchronous methods need
the poll trait in scope.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l accepters Three defects the adversarial review turned up. qmux held an outbound-queue slot across the flow-control wait. The reservation lives in a retained future, so whatever it owns between polls it also owns across an *abandoned* write — and a caller that cancels a write and never returns to the stream keeps it for the life of that stream. The session queue has a handful of slots, so a few such streams block every other stream on the connection. This is distinct from the terminal-transition release already here: nothing ends the stream, so nothing runs that cleanup. The slot is now taken only across a synchronous credit claim and handed back before waiting on the peer's window, so the retained future holds nothing while parked — it retains the waiting, not a resource. Both are still acquired before any bytes leave the caller's buffer. `poll_read_buf` fed `poll_read`'s reported size straight to `BufMut::advance_mut`, which is unsafe while `PollRecvStream` is not, so a safe implementation could advance past the buffer. Assert instead. Panic rather than clamp: an implementation reporting more than the buffer it was handed is broken, and quietly trusting a smaller number would hide it. A count that is wrong but in range stays a plain logic bug — the caller sees zeroed bytes, the usual outcome for a `read` that misreports. iroh and quiche polled one shared accept struct from every clone without keeping the other callers' wakers, so a second concurrent accepter could register only through a waker the first had replaced and never wake. quinn and noq already carry the fix; these never got it. It predates this PR, but the `&mut self` design tells callers to clone for concurrency, which is what makes it reachable — and the iroh comment claimed the wakers were tracked when they were not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
rs/web-transport-quiche/src/connection.rs (1)
592-602: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStray comment now attached to the wrong item.
The "This is poll-based because we accept and decode streams in parallel…" note explains
poll_accept_uni, but it now sits directly aboveremember's doc comment. Move it back down topoll_accept_uni.🤖 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/web-transport-quiche/src/connection.rs` around lines 592 - 602, Move the three-line poll-based/FuturesUnordered comment from above remember to the poll_accept_uni method it describes, keeping remember’s documentation limited to its waker-registration behavior.rs/web-transport-trait/src/lib.rs (1)
78-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a doc comment to the exported
BiStreamsalias.Every other public item in this module is documented; this alias is part of the public surface (it appears in
poll_accept_bi/poll_open_bisignatures).📝 Proposed doc
+/// The `(send, recv)` pair returned when opening or accepting a bidirectional +/// stream on a [`PollSession`]. pub type BiStreams<S> = ( <S as PollSession>::SendStream, <S as PollSession>::RecvStream, );As per coding guidelines: "Document exported Rust and TypeScript APIs".
🤖 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/web-transport-trait/src/lib.rs` around lines 78 - 81, Add a Rust doc comment directly above the exported BiStreams type alias, describing that it represents the send and receive streams returned by bidirectional session operations such as poll_accept_bi and poll_open_bi.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.
Inline comments:
In `@rs/qmux/tests/qmux01.rs`:
- Around line 326-328: Replace the fixed sleep in the qmux test with explicit
synchronization: after the server’s timeout assertion and once the abandoned
accept is parked, send a readiness signal, then await that signal before the
client calls open_uni. Use the test’s existing task/channel coordination
mechanisms and preserve the test’s focus on the other handle’s behavior.
In `@rs/web-transport-trait/src/lib.rs`:
- Around line 419-438: Update the read flow around the visible buf.chunk_mut()
and len handling to detect len == 0 before invoking poll_read, and return
Poll::Ready(Ok(Some(0))) (or the established full-buffer assertion) instead of
allowing the result to be collapsed into EOF. Preserve the existing behavior for
non-empty buffers.
---
Nitpick comments:
In `@rs/web-transport-quiche/src/connection.rs`:
- Around line 592-602: Move the three-line poll-based/FuturesUnordered comment
from above remember to the poll_accept_uni method it describes, keeping
remember’s documentation limited to its waker-registration behavior.
In `@rs/web-transport-trait/src/lib.rs`:
- Around line 78-81: Add a Rust doc comment directly above the exported
BiStreams type alias, describing that it represents the send and receive streams
returned by bidirectional session operations such as poll_accept_bi and
poll_open_bi.
🪄 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: 889172f9-a807-4b71-a498-8296a6b365ad
📒 Files selected for processing (18)
rs/qmux/examples/h3qx.rsrs/qmux/examples/interop-client.rsrs/qmux/examples/interop-server.rsrs/qmux/src/session.rsrs/qmux/src/tcp.rsrs/qmux/tests/backpressure.rsrs/qmux/tests/datagram.rsrs/qmux/tests/negotiation.rsrs/qmux/tests/qmux01.rsrs/qmux/tests/qmux02.rsrs/web-transport-iroh/src/session.rsrs/web-transport-noq/src/session.rsrs/web-transport-quiche/src/connection.rsrs/web-transport-quinn/src/session.rsrs/web-transport-trait/README.mdrs/web-transport-trait/src/lib.rsrs/web-transport-trait/src/util.rsrs/web-transport-trait/tests/defaults.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- rs/qmux/examples/interop-server.rs
- rs/qmux/examples/interop-client.rs
- rs/qmux/tests/backpressure.rs
- rs/qmux/examples/h3qx.rs
- rs/web-transport-trait/tests/defaults.rs
- rs/web-transport-noq/src/session.rs
- rs/web-transport-trait/README.md
- rs/qmux/tests/qmux02.rs
- rs/web-transport-trait/src/util.rs
- rs/web-transport-quinn/src/session.rs
- rs/web-transport-iroh/src/session.rs
- rs/qmux/src/session.rs
…tream `poll_read_buf` and `poll_read_chunk` collapsed "no room to read into" and "peer finished" into the same `None`. The backends already draw that distinction — `(size != 0 || dst.is_empty()).then_some(size)` reports `Some(0)` for an empty slice precisely so a caller can tell them apart — and the defaults threw it away, turning a full buffer into apparent truncation. Also move a stray comment in quiche back onto the method it describes, and replace the sleep in the abandoned-accept test with a signal from the server, so it waits for the accept to actually park rather than guessing at a delay. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Also fixed the stray-comment nitpick in 🤖 Addressed by Claude Code |
Summary
Make
web-transport-traitpoll-native, so consumers such as moq-net can drive thetransport from their own polling loop without duplicating logic between the poll and
async halves.
One rule throughout:
poll_*are the required methods, the async ones are providedhelpers over them. A backend implements the poll surface once.
Both halves split the same way, and the poll traits carry no
Sendbound:PollSendStreamSendStream: PollSendStream + MaybeSendPollRecvStreamRecvStream: PollRecvStream + MaybeSendPollSessionSession: PollSession + MaybeSend + MaybeSyncThe poll traits also hold the synchronous methods (
finish,reset,stop,set_priority,send_datagram,close,protocol,max_datagram_size,stats), soa thread-pinned transport can implement them and be driven from a poll loop without
ever being
Send.PollSession's associated types only require the poll halves, sothe
Sendrequirement doesn't leak in through the stream types either — the wholestack is expressible. Two tests pin that, each implementing the poll surface for a
type holding an
Rc.Backends implement the poll trait and opt in with an empty
impl SendStream for MyStream {}. Overriding still works, and is how quinn, noq and iroh keepwrite_chunk/read_chunkzero-copy — which is why this is a supertrait rather than ablanket impl, since a blanket impl forbids those overrides (
E0119).&mut selfon sessionsSession operations take
&mut self, andCloneis not a supertrait.&mut selfis what makes a retained in-progress operation safe. A&selfpoll methodholding a retained future would have to either share one slot between concurrent
callers — the second polls the first's future with its own waker, so the first hangs
and the resolved stream goes to the wrong task — or clone the session on every call.
&mut selfgives each handle one owner by construction.Concurrency comes from cloning the concrete session; each clone gets independent poll
state. Dropping the
Clonebound means a session that can't be duplicated isexpressible, and an application wanting a freely shared handle opts into wrapping it
rather than every implementation paying for it.
Where a backend already drives an operation from a poll loop it forwards natively:
quinn, quiche, noq and iroh accept through a
FuturesUnordered, and qmux polls itschannels directly. The rest have no poll form anywhere, so
SessionOpsin the traitcrate holds those retained futures — one field per backend instead of six. Those
futures can't borrow the session, so each async body was split into an owned-args form
with the
&selfmethod delegating; qmux instead clones a handle, because its bodiestouch too many fields to thread through as arguments and its clone is refcount bumps
only.
Retention has a cost, paid five times
Retaining a future means holding what it owns for longer than the caller intends.
Every bug found in review was a variant of that, and each has a test that fails
without its fix.
reset,finishand a peer STOP_SENDING left the reservation behind. The queue has 8 slots.
at all: a caller cancels a write, the stream stays open, the slot is gone for its
lifetime. The slot is now taken only across a synchronous credit claim and handed
back before waiting on the peer's window, so the retained future holds nothing while
parked — it retains the waiting, not a resource.
were shared behind a
tokio::sync::Mutex, and the retained future held the guardacross
recv().await. Replaced withSharedRecv: astd::sync::Mutexlocked forthe poll itself, plus a waker list. The bad shape is now unrepresentable — a
std::sync::Mutexguard isn'tSend, so the compiler rejects it.accept struct, which keeps only the most recent waker. quinn and noq already had the
fix; these never got it. It predates this PR, but telling callers to clone for
concurrency is what makes it reachable.
poll_read_buffed an unchecked size toBufMut::advance_mut. That's unsafewhile
PollRecvStreamis not, so a safe implementation could advance past thebuffer. It now asserts — panic rather than clamp, since an implementation reporting
more than the buffer it was handed is broken and quietly trusting a smaller number
would hide it.
Other fixes
write_chunk's default wrote a single partialwrite_bufdespite promising thewhole chunk. Now
write_all_buf.poll_read_buftransmuted anUninitSliceinto a&mut [u8]— UB regardless ofwhether the callee reads it. It now zeroes first (overridable to skip the memset),
and
poll_read_chunkreads into aBytesMut::zeroed, dropping its unsafe entirely.poll_read_buf/poll_read_chunkreported a full destination as end of stream.The backends deliberately distinguish the two (
Some(0)for an empty slice); thedefaults were throwing that away, turning "no room" into apparent truncation.
web_transport_proto::Header, an inline[u8; 16].Two varints is the most a header can be, so it's
Copyand the open path no longerallocates — cloning a
Vecper stream defeated the point of caching the bytes.write/write_buflooped until the buffer drained; they now do a partialwrite like every other backend, which is what the trait has always documented.
Callers wanting the old behavior use
write_all/write_all_buf.write_bufoverrides are gone. They took bytes withcopy_to_bytesbefore the write completed — the hazard quinn's own comment warnsabout. The
write_chunkoverrides, where the caller hands over ownership, stay.Downstream impact
Built moq-net against this branch with
[patch.crates-io]: 227 errors across 12files. 25 are borrow-structural — sessions moved into spawned
'statictasks — andthe rest are impl-signature updates and cascade. That is a deliberate choice to get
the interface right rather than minimize churn in a consumer that is moving to a
poll-based design anyway.
Breaking
Implementors provide
poll_*instead of the async methods, movingtype Errorandthe synchronous methods to the poll traits, then add an empty
impl Session/impl SendStream/impl RecvStream.Sessioncallers hold their session mutably and cloneit for concurrent operations. Callers of the async methods are otherwise unaffected
unless they use the synchronous ones, which need the poll trait imported.
Commits are marked
!withBREAKING CHANGEfooters; release-plz owns the versionbump, so no
Cargo.tomlversion is touched here.Validation
just fix,just check,just test— all greenwasm32, all-features clippy with-D warningsmutation hangs the suite, which is the failure mode it guards
Known gap: the iroh/quiche waker fix has no behavioural test. It mirrors quinn's
proven code and compiles, but a two-cloned-handles test needs a live endpoint per
backend and wasn't built. Of everything here, that change rests on review alone.
(written by Opus 5)