fix(wasm): resubscribe to ready instead of waiting on closed - #374
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Walkthrough
Mergeability Score: ⚪ Minimal · up to The change makes the datagram sender resubscribe to the current readiness signal instead of waiting on an open writer to close, with the reported checks passing; no actionable merge-blocking risk remains beyond normal review. 🚥 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 |
A clone that loses the datagram capacity race holds a `ready` promise that has already fulfilled and never will again, because the writer swaps in a fresh one. Only the current promise can wake it. The `closed` fallback it fell into was unreachable: `ready` and `closed` shared the `Op` slot that `poll_settled` drains at the top of the call, so the loop always started from an empty slot, and a freshly built `JsFuture` polls `Pending` on its first poll. Reaching that arm needed a fulfilled `ready` carried over from an earlier call, which the drain ruled out. It was also wrong on its own terms. Had it become reachable -- by giving the two waits their own slots, the obvious next refactor -- the losing clone would have parked on `closed()` against an open writer, which never fulfills, hanging the send for the life of the session. That is the deadlock `concurrent_datagram_senders` pins. Falling back to an empty slot is what resubscribes today, so drop the arm and `continue`. A writer that closed rather than filled rejects `ready`, so that case still leaves through the error arm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ceea4a0 to
bc8dedb
Compare
|
Adversarially reviewed by Codex against The finding is real but does not block this PR, and is tracked separately as #375. Reasoning: the removed Also worth recording: the obvious repair to that arm — giving Verification: browser harness 14/14 (Chromium, real QUIC peer), including One caveat carried into #375: (written by Opus 5) |
What
Session::poll_send_datagramhad a fallback that waited onwriter.closed()whenreadyhad fulfilled butdesiredSizestill reported no capacity. This removes it andcontinues the loop instead, which resubscribes to the writer's currentreadypromise.Why
Two separate problems, both fixed by the same one-line change.
It was unreachable.
readyandclosedshare the sameOpslot thatpoll_settleddrains at the top of the call, so the loop always starts from an empty slot. A freshly builtJsFuturepollsPendingon its first poll -- wasm-bindgen attachesthencallbacks that fire on a microtask -- so reaching that arm required a fulfilledreadycarried over from an earlier call, which the drain already ruled out. This was verified empirically before removal: the branch was instrumented to set a JS global when entered, and it never fired across the full browser harness, including a hand-driven capacity race.It was also wrong if revived. The natural next refactor -- giving
readyandclosedtheir own slots -- would have made it reachable, and then a clone that lost the capacity race would park onclosed()against an open writer. That promise never fulfills, so the send hangs for the life of the session. That is exactly the deadlockconcurrent_datagram_senderspins, whose doc comment already called it "a deadlock dressed up as backpressure."So the arm was dead code that also modeled a fix that does not work. Removing it leaves the correct behavior the code already relied on: fall back to an empty slot, resubscribe to the current promise.
Where the closed-writer hazard goes
The arm was originally added to stop a closed writer from spinning on an already-fulfilled
ready. That case does not need it: closing the session errors the datagram writable, soreadyrejects, and the error leaves through thePoll::Ready(Err(_))arm. The harness checkclosed datagram writer terminates polldistinguishes this from a spin -- it fails with"closed writer stayed in a wake loop"on timeout -- and still passes.Testing
just harness(Chromium, real QUIC peer): 14 passed, 0 failed, including the two decisive checks --closed datagram writer terminates poll("closed writer returned an error") andconcurrent cloned senders all get capacity("the loser resubscribed and sent").just check: passes.No behavior change for any path that was actually reachable.
Reviewer notes
The reasoning here was challenged adversarially by Codex, which confirmed the unreachability analysis and caught that an earlier version of this change described the branch as becoming "load-bearing" if the slots were split -- it would in fact deadlock. That correction is why the arm is removed rather than documented.
(written by Opus 5)