Skip to content

perf(libs/core): drop libuv-style partial-read break to fix node:http p99 - #33860

Merged
nathanwhit merged 6 commits into
denoland:mainfrom
nathanwhitbot:fix/uv-compat-tcp-tail-latency
May 6, 2026
Merged

nathanwhit merged 6 commits into
denoland:mainfrom
nathanwhitbot:fix/uv-compat-tcp-tail-latency

Conversation

@nathanwhitbot

@nathanwhitbot nathanwhitbot commented May 5, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Bench node:http hello-world at c=100, confirm p99 drops from ~30 ms to ~3 ms
  • Bench Deno.serve hello-world at c=100, confirm no regression
  • CI green

… 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).
@nathanwhit
nathanwhit enabled auto-merge (squash) May 5, 2026 17:40
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.
@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up fix for a regression caught by node compat: test-net-allow-half-open.js was failing with Uncaught Error: write EPIPE.

poll_tcp_handle's MSG_PEEK probe (which detects a broken write side via the read side when reads aren't active) was treating n == 0 (peer FIN) as a broken connection and draining the entire write queue with EPIPE. With the previous behavior of partial-read break, internal_reading rarely became false during normal traffic so this path almost never ran. Removing the break exposed the bug: TCP half-close is valid, and only RST / other recv() errors should fail pending writes.

Now the probe only fails writes on n < 0 (real errors); n == 0 is treated as benign half-close and the EOF is reported through the normal read path. test-net-allow-half-open.js passes, full cargo test --test node_compat shows no other regressions from this change.

2bad5a901

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.js
  • parallel/test-https-max-header-size-per-stream.js
  • parallel/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.

Comment thread libs/core/uv_compat/tcp.rs
Comment thread libs/core/uv_compat/tcp.rs Outdated
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 fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✓ Windows-specific accommodation in 885aa366 is the right call. The reasoning lines up:

  • On Unix, tokio's try_write against a peer that sent FIN-then-RST surfaces the broken-pipe error promptly enough that JS can emit 'error' before 'close'. Treating n == 0 as half-close-still-valid is correct.
  • On Windows, tokio's IOCP-driven try_write_vectored accepts 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 fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✓ 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_write surfaces real errors directly (the in-loop error branch already drains), so the probe is a backstop for the narrow case where readiness is armed but try_write returned WouldBlock instead of an error code.
  • Windows path: tokio's sync send() can silently land bytes in the local kernel send buffer even after peer closesocket() — 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_wouldblock gate: small writes that drain in one shot (the canonical test-net-allow-half-open 3-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.

nathanwhit added 2 commits May 5, 2026 23:03
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.
@nathanwhit
nathanwhit merged commit 3eb1657 into denoland:main May 6, 2026
136 checks passed
littledivy pushed a commit to crowlKats/deno that referenced this pull request Jun 10, 2026
… 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants