perf(node): fire-and-forget response send on the serve() path by pi0 · Pull Request #219 · h3js/srvx

pi0 · 2026-07-02T13:26:16Z

What

The internal serve() request listener no longer tracks per-response end()
completion with a Promise. node:http ignores the listener's return value, so
the tracking Promise (and the microtask hops to settle it) was pure overhead on
the hot path. A new sendNodeResponseDetached variant skips it; streaming
bodies still return their tracking promise since it drives their own cleanup.
The public sendNodeResponse keeps its awaitable contract for toNodeHandler
consumers.

Also collapses the buffered request-body read shared by request.text() and
request.json() behind a private #readBuffered() helper, keeping json()'s
single readBody -> parse continuation (no extra promise/microtask hop).

Safety

Making the send path synchronous removed the implicit try/catch that the old
async sendNodeResponse provided: a synchronous throw during serialization
(e.g. an invalid header value rejected by writeHead) would previously become
a swallowed rejected promise, but on the detached path it escaped the request
listener as an uncaughtException and crashed the process.

sendNodeResponseDetached now guards against this — a serialization throw fails
that single response (500 if headers aren't committed yet, otherwise the socket
is destroyed) instead of taking the server down.

Verification

Full test suite passes (865 passed).
Added runtime check: a forced synchronous writeHead throw is caught, the
process stays up, and the client receives a 500.

Summary by CodeRabbit

New Features
- Added a faster “detached” response path for Node-based requests to better support fire-and-forget behavior.
Bug Fixes
- Improved request body JSON parsing to correctly handle both streamed and buffered inputs.
- Enhanced response sending reliability, including more robust handling of send errors and response completion behavior in Node mode.

coderabbitai · 2026-07-02T13:27:05Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 00a14b39-a08a-4b4c-8858-cd1fb7e0de9a

📥 Commits

Reviewing files that changed from the base of the PR and between ec0d44f and 7af2b9a.

📒 Files selected for processing (2)

src/adapters/_node/request.ts
src/adapters/_node/send.ts

🚧 Files skipped from review as they are similar to previous changes (2)

src/adapters/_node/request.ts
src/adapters/_node/send.ts

📝 Walkthrough

Walkthrough

This PR adds a waitUntil property to the Node Request adapter, changes json() to parse buffered bodies directly when no stream is present, introduces a detached Node response sender with a detached completion path, and switches the Node handler to use that detached sender.

Changes

Node adapter response and request handling

Layer / File(s)	Summary
Request adapter: waitUntil property and json() parsing branch `src/adapters/_node/request.ts`	Declares an optional `waitUntil` property and changes body parsing so `json()` uses buffered bytes directly when `#bodyStream` is absent, while streamed cases still go through `text()`.
Detached send API and endNodeResponse flag threading `src/adapters/_node/send.ts`	Adds `sendNodeResponseDetached`, makes `sendNodeResponse` delegate through shared send logic, threads `detached` through no-body and error paths, updates `endNodeResponse` for void detached completion, and adjusts `pipeBody` error handling typing.
Node adapter wiring to detached sender `src/adapters/node.ts`	Switches the handler import and response path to `sendNodeResponseDetached` for both Promise and non-Promise results, with inline comments about listener return values and end tracking.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

h3js/srvx#183: Related waitUntil surface work; this PR explicitly declares the Node Request adapter property used for later assignment.

Poem

A rabbit hopped through buffer, stream, and send,
With detached replies that reach their end.
waitUntil stands tall, JSON stays neat,
Then node sends free without a repeat.
🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fire-and-forget response sending on the Node serve() path.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch perf/node-perf2

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

pkg-pr-new · 2026-07-02T13:27:33Z

Open in StackBlitz

npm i https://pkg.pr.new/srvx@219

commit: 7af2b9a

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/adapters/node.ts`:
- Around line 87-89: The detached response path in sendNodeResponse can bypass
the existing error handling, so a synchronous failure from writeHead/write or a
rejection in the Promise branch may escape serve() unexpectedly. Update the
sendNodeResponse/sendNodeResponseDetached flow in node.ts so the detached send
is protected by the same try/catch or equivalent guard used by sendNodeResponse,
and ensure the Promise branch handles rejection before calling
sendNodeResponseDetached. Keep the behavior consistent for both sync and async
results.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4932b5e8-311f-4d16-aac1-58137f722240

📥 Commits

Reviewing files that changed from the base of the PR and between 1c7377e and ec0d44f.

📒 Files selected for processing (3)

src/adapters/_node/request.ts
src/adapters/_node/send.ts
src/adapters/node.ts

The internal serve() path switched from the async `sendNodeResponse` to the synchronous `sendNodeResponseDetached`. The async variant implicitly turned a synchronous throw during serialization (e.g. an invalid header value in `writeHead`) into a swallowed rejected promise; the detached variant let it escape the node:http request listener as an `uncaughtException`, crashing the process. Guard `sendNodeResponseDetached` so a serialization throw fails the single response (500 if not yet committed, otherwise destroy the socket) instead of taking the server down. Also dedupe the buffered-body read shared by `text()`/`json()` behind a `#readBuffered()` helper, preserving json()'s single-continuation parse. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

pi0 added 2 commits July 2, 2026 13:24

wip

667032e

Merge branch 'main' into perf/node-perf2

ec0d44f

coderabbitai Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread src/adapters/node.ts

pi0x changed the title ~~perf(node): fire-and-forget response~~ perf(node): fire-and-forget response send on the serve() path Jul 2, 2026

pi0 merged commit 1a9dd8b into main Jul 2, 2026
15 checks passed

pi0 deleted the perf/node-perf2 branch July 2, 2026 13:59

This was referenced Jul 16, 2026

fix(node): Node adapter correctness fixes (statusText, empty body, HEAD streaming, send errors, sync bridge) #243

Merged

fix(node): answer 500 for unhandled handler errors #251

Merged

fix(node): reject body reads after the body is consumed #254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

perf(node): fire-and-forget response send on the serve() path#219

perf(node): fire-and-forget response send on the serve() path#219
pi0 merged 3 commits into
mainfrom
perf/node-perf2

pi0 commented Jul 2, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

pkg-pr-new Bot commented Jul 2, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

pi0 commented Jul 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Safety

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

pkg-pr-new Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pi0 commented Jul 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 2, 2026 •

edited

Loading

pkg-pr-new Bot commented Jul 2, 2026 •

edited

Loading