fix(moq-net): read frames in place, without minting a per-poll FrameConsumer#1736
fix(moq-net): read frames in place, without minting a per-poll FrameConsumer#1736kixelated wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR shifts frame reading from allocating and caching 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/moq-net/src/model/group.rs`:
- Around line 370-383: The issue is that poll_read_frame caches a frame in
self.frame while poll_next_frame ignores this cached state, causing frame
duplication and skipping when APIs are mixed. Fix this by having poll_next_frame
check and return self.frame.take() before resolving a fresh frame, ensuring the
same frame is not processed twice. Additionally, add a regression test that
verifies the Pending to next_frame to complete flow works correctly and doesn't
duplicate or skip frames.
🪄 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
Run ID: c19cd729-d04a-4426-aa6d-4ef246989e61
📒 Files selected for processing (1)
rs/moq-net/src/model/group.rs
| pub fn poll_read_frame(&mut self, waiter: &kio::Waiter) -> Poll<Result<Option<Bytes>>> { | ||
| let Some(mut frame) = ready!(self.poll(waiter, |state| state.poll_get_frame(self.index))?) else { | ||
| return Poll::Ready(Ok(None)); | ||
| }; | ||
| if self.frame.is_none() { | ||
| let index = self.index; | ||
| let Some(frame) = ready!(self.poll(waiter, |state| state.poll_get_frame(index))?) else { | ||
| return Poll::Ready(Ok(None)); | ||
| }; | ||
| self.frame = Some(frame); | ||
| } | ||
|
|
||
| let data = ready!(frame.poll_read_all(waiter))?; | ||
| let data = ready!(self.frame.as_mut().unwrap().poll_read_all(waiter))?; | ||
| self.frame = None; | ||
| self.index += 1; | ||
|
|
||
| Poll::Ready(Ok(Some(data))) |
There was a problem hiding this comment.
Coordinate next_frame with the cached in-flight frame.
Persisting self.frame here makes GroupConsumer stateful across polls, but poll_next_frame still ignores that state. A reachable sequence is: poll_read_frame* caches frame N and returns Pending, next_frame then resolves frame N again and bumps index, and when the cached read finally completes it bumps index a second time. That duplicates frame N and skips frame N + 1.
Either have poll_next_frame return self.frame.take() before resolving a fresh frame, or explicitly reject switching APIs while frame.is_some(). Please add a regression test for the Pending -> next_frame -> complete path with the fix.
Suggested direction
pub fn poll_next_frame(&mut self, waiter: &kio::Waiter) -> Poll<Result<Option<FrameConsumer>>> {
- let Some(frame) = ready!(self.poll(waiter, |state| state.poll_get_frame(self.index))?) else {
- return Poll::Ready(Ok(None));
- };
+ let frame = if let Some(frame) = self.frame.take() {
+ frame
+ } else {
+ let Some(frame) = ready!(self.poll(waiter, |state| state.poll_get_frame(self.index))?) else {
+ return Poll::Ready(Ok(None));
+ };
+ frame
+ };
self.index += 1;
Poll::Ready(Ok(Some(frame)))
}Also applies to: 392-405
🤖 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/moq-net/src/model/group.rs` around lines 370 - 383, The issue is that
poll_read_frame caches a frame in self.frame while poll_next_frame ignores this
cached state, causing frame duplication and skipping when APIs are mixed. Fix
this by having poll_next_frame check and return self.frame.take() before
resolving a fresh frame, ensuring the same frame is not processed twice.
Additionally, add a regression test that verifies the Pending to next_frame to
complete flow works correctly and doesn't duplicate or skip frames.
…onsumer GroupConsumer::poll_read_frame / poll_read_frame_chunks called poll_get_frame -> frame.consume() on every poll, then dropped that FrameConsumer whenever the frame's data wasn't complete yet (still in flight). A FrameConsumer is a kio consumer handle, so that create+drop flips the frame's consumer count 0->1->0 each poll, and kio wakes the state's waiters on both the first-appears and last-drops transitions -- the same waiters our own read registered on. Every poll re-woke itself: a silent busy spin. On a multi-threaded runtime the producer fills the frame concurrently so the spin ends in microseconds (wasted CPU, no visible hang). On a single-thread executor (e.g. wasm) the consumer's self-wake loop starves the producer, so the frame never completes and the spin runs away into a hard freeze. Read the frame in place instead of through a consumer handle: - kio: add `Producer::poll_ref`, a read-only counterpart to `Producer::poll` that registers a waiter on a read condition without taking a `Mut` (no modified flag, no consumer-count churn). - model/frame: `FrameProducer::poll_read_all` reads the producer's own buffer once finished, via poll_ref. Stateless (always offset 0), so parallel readers are fine. - model/group: `GroupState::poll_frame_read_all` reads the cached FrameProducer directly; poll_read_frame / poll_read_frame_chunks use it and no longer mint a FrameConsumer. GroupConsumer stays a plain derive(Clone) with no extra state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kixelated
force-pushed
the
fix-group-consumer-wake-spin
branch
from
5aee99f to
ea13e11
Compare
kixelated
changed the title
|
Superseded by the kio fixes #1735 (read-only (Written by Claude) |
1 participant
Summary
GroupConsumer::poll_read_frame/poll_read_frame_chunkscalledpoll_get_frame→frame.consume()on every poll, then dropped thatFrameConsumerwhenever the frame's data wasn't complete yet (poll_read_allstillPending). AFrameConsumeris a kio consumer handle, so that create+drop flips the frame's consumer count0→1→0each poll, and kio wakes the state's waiters on both the "first consumer appears" and "last consumer drops" transitions — the same waiters the read itself registered on. So every poll re-woke itself — a silent busy spin.@moq/net-over-WASM consume path ondev) the consumer's self-wake loop starves the producer, so the frame never completes and the spin runs away into a hard browser freeze (~22M re-polls / ~45M wakes on one frame).Fix
Read the frame in place instead of through a consumer handle:
Producer::poll_ref— a read-only counterpart toProducer::pollthat registers a waiter on a read condition without taking aMut(nomodifiedflag, no consumer-count side effects).model/frame:FrameProducer::poll_read_allreads the producer's own buffer once the frame is finished, viapoll_ref. Stateless (always offset 0), so any number of parallel readers are fine.model/group:GroupState::poll_frame_read_allreads the cachedFrameProducerdirectly;poll_read_frame/poll_read_frame_chunksuse it and no longer mint aFrameConsumer.GroupConsumerstays a plainderive(Clone)with no extra state.Public API
GroupConsumerunchanged. One additive kio method (Producer::poll_ref).FrameProducer::poll_read_allispub(crate).Test plan
cargo test -p moq-net --lib— 346 pass;cargo test -p kiocargo clippy -p moq-net -p kioclean,cargo fmtdevwasm PR (feat(wasm): @moq/wasm as a drop-in for @moq/net; flip watch/publish/boy #1726) carries the same fix and is where the freeze was first observed.🤖 Generated with Claude Code
(Written by Claude)