Repository navigation
fix(ext/websocket): don't panic on H2 stream reset in poll_write - #33982
Conversation
The H2 arm of `poll_write` ignored the result of `poll_capacity` — when the peer sends `RST_STREAM`, `poll_capacity` returns `Ready(None)`, `send.capacity()` returns 0, and `assert!(size > 0)` panicked the process. Match on the `poll_capacity` result and propagate the closed-stream state as a regular write error (`io::ErrorKind::ConnectionReset` / `WriteZero`). `ws.send()` now surfaces a JS NetworkError instead of aborting the runtime. The reporter (Kimi K2.6 under human oversight) provided a downloadable repro at https://github.com/user-attachments/files/27574883/h2-ws-panic-repro.zip that reliably exercises the bug; the same script exits cleanly after this change. A unit/spec test would require an H2 server that can inject a RST_STREAM mid-write, which the existing WS test harness doesn't have today. Leaving that as a follow-up; happy to add a sidecar Rust h2 server in a separate PR if you'd like. Fixes denoland#33953. Signed-off-by: lunadogbot <[email protected]>
`send.poll_capacity()` returns `Poll<Option<Result<usize, h2::Error>>>` — the value inside `Ok` is the reserved-capacity count, not `()`. Use `Some(Ok(_))` so the pattern actually compiles. Signed-off-by: lunadogbot <[email protected]>
Deno Individual Contributor License AgreementAll contributors have signed the CLA. Thank you! This is an automated message from CLA Assistant |
|
CI is green (134 success, 2 skipped, 0 failures). Ready for review when you have a moment, @bartlomieju. Same CLA blocker as #33978 and #33980: |
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
The three-arm match on send.poll_capacity(cx) matches h2's contract — Some(Ok) continues, Some(Err) and None (peer RST_STREAM) both surface as ConnectionReset instead of the prior assert!(size > 0) panic. The post-match size == 0 guard at line 127 is belt-and-suspenders for a concurrent close between successful capacity reservation and send.capacity() — harmless to keep.
No regression test, but the description's reasoning (RST_STREAM injection mid-write needs new H2 server test infrastructure) is sound; follow-up PR is fine.
CI green (134 SUCCESS / 2 SKIPPED).
|
@bartlomieju this is ready to merge |
3 participants
The H2 arm of
poll_writeinext/websocket/stream.rs(RFC 8441 WebSockets over HTTP/2) ignored the result ofpoll_capacity— when the peer sendsRST_STREAM,poll_capacityreturnsReady(None),send.capacity()returns 0, and the nextassert!(size > 0)panicked the process. The previous comment (// TODO(mmastrac): the documentation is not entirely clear what to do here, so we'll continue) flagged the gap; this PR resolves it.After this change, the same RST_STREAM scenario surfaces as a regular write error (
io::ErrorKind::ConnectionResetfor theSome(Err)/Nonepaths,WriteZeroifcapacity()would otherwise be 0).ws.send()then bubbles up as a JSNetworkErrorinstead of aborting the runtime.The reporter (KTibow / Kimi K2.6 under human oversight) shipped an excellent diagnosis with a downloadable repro zip — a Rust h2 server + Deno client that reproduces the panic in seconds. The same script exits cleanly after this change.
No regression test in this PR: a unit/spec test would require an H2 server that injects a
RST_STREAMmid-write, which the existingtests/specs//tests/unit/websocket_test.tsharnesses don't currently support without a sidecar Rust binary. Happy to add that in a follow-up PR if you'd like.deno fmt --check/deno lintaren't useful on a.rsfile; the worker host doesn't haverustfmtavailable so the Rust formatting will get its first authoritative check from CI.Credit to @KTibow for the diagnosis, the verbatim-correct suggested fix, and the reproducer.
Fixes #33953.
@bartlomieju
🤖 Filed by
agor(workerwork2-2, botlunadogbot). Tracking claim: bartlomieju/agent-job-board#26. (Note:@lunadogbotstill needs CLA acceptance from the operator side, same as #33978 and #33980.)