Repository navigation
perf(libs/core): drop libuv-style partial-read break to fix node:http p99 - #33860
Conversation
… p99 `poll_tcp_handle`'s read loop matched libuv's `uv__read` by breaking on partial reads (`n < buf.len`) to skip the predicted-EAGAIN syscall. That's correct for libuv's epoll loop, but wrong against tokio: tokio's `TcpStream` readiness is edge-triggered and the internal "readable" bit is cleared only by a `WouldBlock` from `try_read`. Breaking on a partial read left the bit armed, the re-register block at the end of the function then saw `poll_read_ready == Ready`, and `mark_ready()` put the handle right back on the loop's ready queue and woke `loop_waker`. Under sustained HTTP keep-alive load with small responses every read is partial, so every tick re-queued every active connection. Tokio's current_thread runtime saw the deno task continuously awoken and never yielded to its reactor (controlled by `event_interval`, default 61). New TCP readiness from the kernel piled up undelivered and each connection waited a full event_interval window before its next request was processed. The fix is one line: keep looping until `try_read` actually returns `WouldBlock`. That costs one extra `recv()` syscall when the read fully drains the socket buffer, but the syscall returns `WouldBlock`, clears tokio's readiness bit, and the handle drops out of the ready queue cleanly. The reactor is then free to deliver fresh wakes. 100-connection oha bench against `node:http` hello-world (`release-lite`): | | node | deno before | deno after | | ------- | ------- | ----------- | ---------- | | p50 | 1.26 ms | 0.96 ms | 1.58 ms | | p99 | 2.56 ms | 33.4 ms | 3.14 ms | | p99.9 | 4.26 ms | 60.7 ms | 6.34 ms | | p99.99 | 7.28 ms | 68.0 ms | 8.28 ms | `Deno.serve` p99 unchanged (1.31 ms; no regression).
The MSG_PEEK probe in `poll_tcp_handle`'s write path fired when reads weren't active and treated `n == 0` (peer FIN) the same as a read error: it drained the entire write queue with EPIPE. Before the partial-read break was removed, `internal_reading` rarely became false during normal traffic so this path almost never ran. Removing the partial-read break (so the read loop runs to WouldBlock and clears `internal_reading` cleanly on EOF) exposed the bug: test-net-allow-half-open's second case writes after the peer half- closes, and now hit `Uncaught Error: write EPIPE`. Treating EOF as "connection broken" is wrong. TCP half-close is valid: a peer can send FIN and still keep its read side open. Only RST or other recv() errors mean the connection is unusable for writes. The probe still detects those — `n < 0` with a non-WouldBlock errno still drains pending writes with EPIPE — but `n == 0` is now treated as benign half-close. The actual EOF is reported through the normal read path.
|
Pushed a follow-up fix for a regression caught by node compat:
Now the probe only fails writes on |
fibibot
left a comment
There was a problem hiding this comment.
Excellent diagnosis on the read-loop fix. The walk-through (libuv's uv__read partial-read break + tokio's edge-triggered readiness + self-wake loop + event_interval starvation) is exactly right, and the p99 numbers (33ms→3ms at c=100) are a meaningful node:http perf win. The extra recv() syscall to clear tokio's internal readiness bit is the correct cost.
Two-commit structure I want to acknowledge
The earlier CI run on 9b3c3261 (read-loop fix only) failed 22 shards with Uncaught Error: write EPIPE on three specific tests:
parallel/test-net-allow-half-open.jsparallel/test-https-max-header-size-per-stream.jsparallel/test-https-insecure-parse-per-stream.js
Follow-up commit 2bad5a90 (fix(libs/core): don't fail pending writes on read EOF) appears to correctly diagnose the cause: the read-loop fix made internal_reading=false happen reliably on peer FIN (because we now drain to EOF in one tick), which in turn lets the post-loop MSG_PEEK branch fire, which previously drained the write queue with UV_EPIPE on n==0 — incorrectly conflating peer FIN with broken connection. The OLD code had this same bug latent; it was masked by the OLD read loop's break-on-partial leaving internal_reading=true so the MSG_PEEK branch rarely fired in steady state.
The fix in commit 2 (treat n==0 as legitimate half-close, only n<0 as hard error) is correct per TCP semantics — half-close is valid (peer stopped sending but is still receiving), and EOF is properly handled through the normal read path's Ok(0) → UV_EOF callback.
CI re-confirm
CI is currently rebuilding at this HEAD (55 pass, 12 pending, 0 fail so far). Will re-confirm once test integration and test node_compat shards finish — the same shards that were red on commit 1 are exactly what needs to land green to validate the fix.
Not APPROVE-ing yet per the CI-must-be-green rule; substance is correct as far as I can verify and the regression-handling commit looks right. One inline observation on the new code shape.
Reverts the n=0 portion of the prior fix on Windows only. libuv on Windows uses async writes via WSASend with overlapped I/O; uv__process_tcp_write_req surfaces broken-pipe errors through the IOCP completion. tokio's try_write_vectored (used by our uv_compat write loop) is synchronous send(): when the peer is gone, the local kernel can still queue bytes and report success, and the broken state only surfaces on a later send/recv after the OS notices the RST. By the time it does, the test's auto-shutdown after read EOF has already closed the socket and the process has exited without raising the expected 'error' event (test-net-error-twice.js relies on this signal). Mimic libuv's IOCP-driven error delivery on Windows by keeping the n == 0 → drain-with-EPIPE branch of the existing MSG_PEEK probe. On unix, n == 0 (peer FIN) remains a benign half-close — tokio's send returns ECONNRESET / EPIPE directly when the connection is actually broken.
fibibot
left a comment
There was a problem hiding this comment.
✓ Windows-specific accommodation in 885aa366 is the right call. The reasoning lines up:
- On Unix, tokio's
try_writeagainst a peer that sent FIN-then-RST surfaces the broken-pipe error promptly enough that JS can emit'error'before'close'. Treatingn == 0as half-close-still-valid is correct. - On Windows, tokio's IOCP-driven
try_write_vectoredaccepts bytes into the local kernel queue and the broken-pipe error doesn't surface until much later. By then the test's auto-shutdown after read EOF has already fired'close'and the test has exited without ever raising the expected'error'event. (Cited test:test-net-error-twice.js.)
Keeping the n == 0 → drain UV_EPIPE path under #[cfg(windows)] matches what libuv's IOCP backend effectively does — eager error delivery to compensate for IOCP not surfacing it on the write side. The Unix path stays clean (let try_write report breakage when it actually breaks), preserving correct half-open semantics.
CI rebuilding: 58 pass, 29 pending, 0 fail. Will re-confirm once the Windows-specific shards (the ones that were red on commit 2) land — those are exactly what this commit needs to validate.
Reorder poll_tcp_handle so the write loop runs before the broken-connection probe, and only run the probe when the write loop actually hit WouldBlock with a non-empty residual queue. Small writes queued behind a peer FIN (e.g. test-net-allow-half-open's 3-byte 'asd') drain in one pass and skip the probe entirely, while large stuck queues whose peer is gone (test-net-error-twice's 10MB write on Windows) still get the EPIPE drain they need.
fibibot
left a comment
There was a problem hiding this comment.
✓ Cleaner design in 4464d8de. Moving the MSG_PEEK probe AFTER the write-loop and gating on hit_wouldblock && !queue.empty() && !internal_reading && read_ready is the right shape — it sidesteps the over-firing pattern that previously broke test-net-allow-half-open.
The explicit reasoning in the new comment block is exactly what the audit needs:
- Unix path:
try_writesurfaces real errors directly (the in-loop error branch already drains), so the probe is a backstop for the narrow case where readiness is armed buttry_writereturned WouldBlock instead of an error code. - Windows path: tokio's sync
send()can silently land bytes in the local kernel send buffer even after peerclosesocket()— libuv's IOCP backend avoids this via overlapped completions but we don't have that path here, so the probe is what surfaces the broken-pipe error in time for the JS layer to emit'error'before the auto-shutdown fires'close'. hit_wouldblockgate: small writes that drain in one shot (the canonicaltest-net-allow-half-open3-byte 'asd' case) skip the probe entirely. Half-close stays valid.
CI rebuilding: 55 pass, 12 pending, 0 fail. The relevant integration shards (linux/macos × debug + the Windows-specific tests like test-net-error-twice.js) are exactly what this commit needs to validate. Will re-confirm once they land.
Nice iteration on this — the perf win (33ms→3ms p99) is preserved through three commits of getting the half-close semantics right.
Use a per-connection "ever received data" flag as the probe gate, replacing the WouldBlock + !internal_reading combo. This distinguishes the test-net-allow-half-open scenario (peer sends 1024 bytes before FIN — received_data=true, probe skipped, queued 'asd' write completes) from test-net-error-twice (peer destroys without writing — received_data stays false, probe fires). On Windows, also retroactively flip the most recent successful write to EPIPE when the probe trips. Windows' kernel send buffer can absorb large writes silently (auto-tuning grows it past 10 MB on loopback), so the queue may have already drained to "successful" by the time the probe runs — without the flip, the broken-connection error libuv would have reported via overlapped IOCP completion is lost entirely.
Replace the multi-commit churn from 2bad5a9..9021fc5 with a single one-line gate. The earlier perf fix (9b3c326) made the read loop drain to WouldBlock instead of breaking on a partial read, which means EOF can now be observed in the same poll as the preceding data byte. The original broken-connection probe gated on `!internal_reading`, so once that flag flipped on the same-poll EOF, the probe drained legitimate queued writes (test-net-allow- half-open's 'asd') with EPIPE. Track a per-connection `internal_received_data` flag that flips true on the first non-zero read and persists. Add it as an extra gate on the probe: skip it when the peer has ever sent us data (half-close is legitimate — write side stays valid), run it when the peer FIN'd without writing (test-net-error-twice — writes can't be delivered). This restores the pre-2bad5a901 probe semantics for the "peer-aborted-without-writing" case (drain queue with EPIPE on n==0 / n<0) while leaving the "peer-FIN'd-after-writing" case alone, fixing both tests on Linux and Windows without the order / hit_wouldblock / flip-completed-writes contortions of the intermediate commits.
… p99 (denoland#33860) ## Summary `poll_tcp_handle`'s read loop in `uv_compat` matches libuv's `uv__read` by breaking on partial reads (`n < buf.len`) to skip the predicted-EAGAIN syscall. That works for libuv's epoll loop, but it interacts badly with tokio: tokio's `TcpStream` readiness is edge-triggered and the internal "readable" bit is cleared only when `try_read` returns `WouldBlock`. Breaking on a partial read leaves the bit armed; the re-register block at the end of the function then sees `poll_read_ready == Ready` and `mark_ready()` puts the handle right back on the loop's ready queue and wakes `loop_waker`. Under sustained HTTP keep-alive load with small responses every read is partial, so every tick re-queues every active connection. Tokio's current_thread runtime sees the deno task continuously awoken and never yields to its reactor (controlled by `event_interval`, default 61). New TCP readiness from the kernel piles up undelivered, and each connection waits a full event_interval window before its next request is processed. The fix is to keep looping until `try_read` actually returns `WouldBlock`. That costs one extra `recv()` syscall when the read fully drains the socket buffer, but the syscall returns `WouldBlock`, clears tokio's readiness bit, and the handle drops out of the ready queue cleanly so the reactor is free to deliver fresh wakes. This was easy to confirm with `DENO_TOKIO_EVENT_INTERVAL=1`, which forces tokio to check the reactor every poll and made p99 drop from 33 ms to 3 ms even without the source change. ## Reproducer \`\`\`js // server.mjs import { createServer } from "node:http"; const port = Number(process.env.PORT ?? "8004"); createServer((_req, res) => { res.writeHead(200, { "content-type": "text/plain" }); res.end("Hello, World!"); }).listen(port, "127.0.0.1"); \`\`\` \`\`\` oha -c 100 -z 20s --no-tui http://127.0.0.1:8004/ \`\`\` ## Results (release-lite, 100 connections, 20 s) | | node | deno before | deno after | | ------ | -------- | ----------- | ---------- | | p50 | 1.26 ms | 0.96 ms | 1.58 ms | | p99 | 2.56 ms | 33.4 ms | 3.14 ms | | p99.9 | 4.26 ms | 60.7 ms | 6.34 ms | | p99.99 | 7.28 ms | 68.0 ms | 8.28 ms | \`Deno.serve\` p99 is unchanged at 1.31 ms (no regression on the hyper path). ## Test plan - [x] Bench `node:http` hello-world at `c=100`, confirm p99 drops from ~30 ms to ~3 ms - [x] Bench `Deno.serve` hello-world at `c=100`, confirm no regression - [ ] CI green Co-authored-by: Nathan Whitaker <[email protected]>
Summary
poll_tcp_handle's read loop inuv_compatmatches libuv'suv__readby breaking on partial reads (n < buf.len) to skip the predicted-EAGAIN syscall. That works for libuv's epoll loop, but it interacts badly with tokio: tokio'sTcpStreamreadiness is edge-triggered and the internal "readable" bit is cleared only whentry_readreturnsWouldBlock. Breaking on a partial read leaves the bit armed; the re-register block at the end of the function then seespoll_read_ready == Readyandmark_ready()puts the handle right back on the loop's ready queue and wakesloop_waker.Under sustained HTTP keep-alive load with small responses every read is partial, so every tick re-queues every active connection. Tokio's current_thread runtime sees the deno task continuously awoken and never yields to its reactor (controlled by
event_interval, default 61). New TCP readiness from the kernel piles up undelivered, and each connection waits a full event_interval window before its next request is processed.The fix is to keep looping until
try_readactually returnsWouldBlock. That costs one extrarecv()syscall when the read fully drains the socket buffer, but the syscall returnsWouldBlock, clears tokio's readiness bit, and the handle drops out of the ready queue cleanly so the reactor is free to deliver fresh wakes.This was easy to confirm with
DENO_TOKIO_EVENT_INTERVAL=1, which forces tokio to check the reactor every poll and made p99 drop from 33 ms to 3 ms even without the source change.Reproducer
```js
// server.mjs
import { createServer } from "node:http";
const port = Number(process.env.PORT ?? "8004");
createServer((_req, res) => {
res.writeHead(200, { "content-type": "text/plain" });
res.end("Hello, World!");
}).listen(port, "127.0.0.1");
```
```
oha -c 100 -z 20s --no-tui http://127.0.0.1:8004/
```
Results (release-lite, 100 connections, 20 s)
`Deno.serve` p99 is unchanged at 1.31 ms (no regression on the hyper path).
Test plan
node:httphello-world atc=100, confirm p99 drops from ~30 ms to ~3 msDeno.servehello-world atc=100, confirm no regression