node:http: pause the socket on Windows too when the request body is paused - #37977
Conversation
…aused do_pause re-armed onData to buffer body chunks natively but skipped pause_socket() under cfg(not(windows)), so on Windows a paused request kept reading from the kernel at full speed and the body accumulated in buffered_request_body_data_during_pause until resume. The guard dates from when the libuv backend could not report a peer FIN on a socket polling without READABLE; the backend now arms UV_DISCONNECT on every poll and probes paused sockets, and every terminal path here resumes the socket first, so the pause is safe on every platform. Adds request-body backpressure tests (http and https) that upload 32 MiB into a paused request and require the client to stall, then drain.
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: merged as a28fcc4. Reproduced on Windows x64 with the shipped canary ( The stall tests added to |
There was a problem hiding this comment.
LGTM — removes a stale #[cfg(not(windows))] guard so do_pause reaches TCP on Windows, matching the behavior Linux/macOS already have.
What was reviewed:
- Verified the guard's original rationale is obsolete:
packages/bun-usockets/src/eventing/libuv.cnow armsUV_DISCONNECTunconditionally andpoll_cbMSG_PEEK-probes paused sockets, exactly as the PR describes. - Confirmed
pause_socket_reads(pipelining flood guard) already callsraw.pause()on every platform with no Windows exclusion, anddo_resumecallsresume_socket()before any flag checks — so paused sockets are always re-armed. - Tests use bounded polling (12×25ms) to detect the stall, cover http+https and both explicit/implicit pause paths, and clean up via
await using/try-finally.
Extended reasoning...
Overview
The Rust change is two deletions in src/runtime/server/NodeHTTPResponse.rs: the #[cfg(not(windows))] guard around self.pause_socket() in do_pause, and the #[allow(dead_code)] on pause_socket that existed only because the guard made it dead on Windows. Compiled output on Linux/macOS is byte-identical; only Windows changes. The test change adds a request body describe group to test/js/node/http/node-http-backpressure.test.ts with 5 tests (2 stall scenarios × http/https, plus a FIN-while-paused deferred-EOF test).
Security risks
None. This adjusts read-side flow control on the server socket; no auth, crypto, parsing, or trust-boundary changes.
Level of scrutiny
The native change is a stale-TODO removal whose justification is directly verifiable in-tree: I confirmed libuv.c now unconditionally arms UV_DISCONNECT in us_poll_start/us_poll_change and probes paused sockets with MSG_PEEK to distinguish FIN from RST — the exact machinery the TODO was waiting on. I also confirmed the same raw.pause() primitive is already live on Windows via pause_socket_reads (no cfg guard there), and that do_resume unconditionally calls resume_socket() before its flag checks, so a paused socket is always re-armed on resume/end/abort. The risk surface is Windows-only and the PR reports 497/498 upstream test-http* files pass identically before and after.
Other factors
The PR description is unusually thorough: it traces the guard's history, cites the PR that made it obsolete, provides before/after RSS measurements on Windows, and reports fail-before/pass-after on the 4 stall tests. The new tests follow the file's existing conventions (raw net/tls sockets, once for readiness, await using servers, port 0, try/finally socket cleanup), use bounded polling rather than blind sleeps to detect the stall, wire aborted to reject and pre-attach .catch(() => {}) to avoid unhandled rejections, and the 32 MiB payload gives ample margin over the ~3 MB measured stall point. No CODEOWNERS cover these paths and there are no outstanding reviewer comments.
Problem
node:httpserver whose handler stops reading the request body (req.pause(), or simply not consumingreq) keeps accepting the upload at full speed; the bytes pile up in native memory until the request is resumed. Same scenario as fetch() with ReadableStream request body ignores backpressure on Windows #26332, which was filed from Windows: node:http: emit 'pause' on req.socket once an unread body fills the IncomingMessage buffer #34740 bounded the JS-sideIncomingMessagebuffer on every platform, but on Windows that only moved the growth into the native pause buffer.req.pause()(loopback upload of 256 KiB chunks): Windows x64 canary9a543cc18has pulled 8195 chunks (2 GB) and RSS is 2.1 GB and climbing; Linux stays at 12 chunks and 37 MB; Node v26 on Windows stays at 15 chunks.'end'never fired before the FIN arrived); Node and Bun on Linux deliver the body and the response.NodeHTTPResponse::do_pause(src/runtime/server/NodeHTTPResponse.rs) re-arms uWSonDatawithon_buffer_paused_shim, which appends every chunk tobuffered_request_body_data_during_pausewith no bound, but theself.pause_socket()call that stops the kernel reads was under#[cfg(not(windows))](// TODO: figure out why windows is not emitting EOF with UV_DISCONNECT). Every pause path (req.pause(), thepush() === false->readStop(socket)path from node:http: emit 'pause' on req.socket once an unread body fills the IncomingMessage buffer #34740,req.socket.pause()) ends indo_pause, so none of them reached TCP on Windows.Fix
do_pausecallspause_socket()on every platform (andpause_socketloses the#[allow(dead_code)]that existed only because it was dead on Windows). No other code changes.EPOLLRDHUP|EPOLLHUP|EPOLLERR, a keptEVFILT_WRITE) but had no equivalent for the libuv backend. node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 added that equivalent:us_poll_start/us_poll_changeinpackages/bun-usockets/src/eventing/libuv.calways armUV_DISCONNECT,poll_cbprobes a paused socket withMSG_PEEKto tell a graceful FIN (deferred until resume) from a reset (closed immediately), and the shared dispatch inloop.cdefers EOF for a paused socket until it resumes. The symptom the TODO names is exactly what node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 fixed.node:httpsocket always gets resumed:do_resumecallsresume_socket()before any flag checks, andend(),writeHeadAndEndandabort()resume the socket first as well, so a response ending with an unread body (req._dump()afterres.end()) or a teardown re-arms the poll and any deferred FIN is delivered. This is the behavior Linux and macOS have had all along; this change gives Windows the same one.Bun.serverequest-body backpressure (Bun.serve: apply TCP backpressure to a request body the handler reads slowly #36006,RequestContext::pause_request_body_socket) and the node:http pipelining flood guard (pause_socket_reads) call the sameuws_res_pause->us_socket_pauseon every platform.test/js/node/http/node-http-backpressure.test.ts, newrequest bodygroup. Each stall test uploads a 32 MiB body into a request that is paused (explicitly, or implicitly by never being read) and requires the client's upload to stall short of the total, then resumes and requires all 32 MiB plus a 200 response; run over both http and https. A fifth test sends a small body plus FIN while the request is paused and requires them to be delivered on resume.Expected: < 33554432, Received: 33554432); with the fix the whole file passes (19/19). The FIN test passes on both and is coverage for the newly enabled deferred-EOF path, not the fail-before proof.test-http-*/test-https-*files fromtest/js/node/test/parallelwith the debug build: 497 pass both before and after. The one failure (test-http-set-timeout-server.js, a 1 msserver.setTimeoutfiring twice) is identical before and after and passes on the release canary.test/js/node/httpdirectory with the fix: 756 pass, 23 skip, 4 todo, 0 fail.Background
us_socket_pausedrops the socket's readable interest in the event backend (epoll/kqueue on POSIX, libuvuv_pollon Windows); the kernel receive buffer then fills, the peer's send window closes, and its writes block. That is how read-side backpressure reaches a TCP peer.us_socket_resumere-adds the interest.on_endafter the socket resumes. A reset closes the socket right away. The libuv backend needs extra machinery for this because Windows AFD only reports a FIN to a poll without read interest through the one-shotUV_DISCONNECTevent; that machinery is what node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 added.on_buffer_paused_shim/buffered_request_body_data_during_pause: while a node:http request is paused, body chunks that uWS has already read are parked in thisVecand handed to JS as oneBufferon resume. With the socket actually paused it holds at most what was already in flight (one recv buffer); without the pause it held the rest of the upload.Repro script and measurements
Windows x64, release canary
1.4.0-canary.1+9a543cc18(unfixed):Windows x64, debug build of this branch's parent (unfixed):
Windows x64, debug build with this change:
Linux, canary
da3851e57(unchanged by this PR):pullsstays at 12, RSS 37 MB.