perf(run): write Marko HTML renders straight to the Node socket - #214
Conversation
🦋 Changeset detectedLatest commit: 17412f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe runtime stores async render output on responses, creates lazy response bodies, and handles iterator cleanup failures during cancellation. Node middleware retrieves valid raw renders or falls back to 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
1b69061 to
e8e7ad9
Compare
4b80030 to
9d66089
Compare
The Node adapter writes a page render's HTML strings directly to the response, skipping the whatwg ReadableStream/TextEncoder round-trip. The public API is unchanged and other adapters still read response.body. The render is carried on the Response under a `Symbol.for` registry key: the runtime and the node adapter are built as separate esbuild bundles, so a module-scoped symbol (or a WeakMap) would be duplicated and never match. A render result is single-use, so its iterator is created eagerly (marko attaches error handling there, and a lazy body would let a failed render throw uncaught) and is then the only consumer of the render. The adapter only takes the shortcut while the response still holds the body the render was stashed with, since `clone()` tees that body and drains the render.
`context.render` now makes a single `Symbol.asyncIterator` check with two code paths: renders that cannot be iterated return a `toReadable` response immediately; iterable renders build the lazy body and stash the raw render for byte-sink adapters. Behavior is unchanged. The adapter-middleware test fixture hoists its TextEncoder and uses one streaming TextDecoder per collect instead of allocating a decoder per chunk, which skewed measurements and mis-decodes multi-byte characters split across chunk boundaries. The fixture body now also enqueues per chunk like a real streamed render. scripts/bench-render-body.ts measures the `getRender` shortcut against reading `response.body`, end to end over HTTP through the real runtime and middleware. On node 24 the direct write sustains ~1.4-1.6x the request throughput of the WHATWG body read across 4KB/64KB/256KB pages. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UbmGqH57TZSYCfKbBtcDi
9b4c201 to
3d9ba9d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/run/src/__tests__/adapter-middleware.test.ts (1)
63-89: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a test for the eager-iterator error-handling rationale.
These cases cover stash/fallback/clone/read well, but none exercise the specific justification documented in
runtime/internal.tsfor creating the iterator eagerly ("a render that fails before anything reads it would otherwise throw uncaught"). A test with a render that rejects before its stream is ever pulled (simulating a HEAD-like unread body) would directly validate that rationale and guard against regressions if the eager-creation logic is ever refactored away.🤖 Prompt for 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. In `@packages/run/src/__tests__/adapter-middleware.test.ts` around lines 63 - 89, Add a getRender test using a render that rejects immediately before the response body is read, simulating an unread HEAD-like body. Assert the rejection is handled without an uncaught error, validating the eager iterator creation rationale documented in runtime/internal.ts while preserving the existing stash and fallback cases.
🤖 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 `@packages/run/src/runtime/internal.ts`:
- Around line 95-120: Update toResponseBody’s ReadableStream cancel handler so
any promise returned by iterator.return?.(reason) is returned or awaited by the
cancellation machinery, allowing rejection to be propagated instead of
discarded. Preserve the existing optional-return behavior when no
iterator.return method exists.
---
Nitpick comments:
In `@packages/run/src/__tests__/adapter-middleware.test.ts`:
- Around line 63-89: Add a getRender test using a render that rejects
immediately before the response body is read, simulating an unread HEAD-like
body. Assert the rejection is handled without an uncaught error, validating the
eager iterator creation rationale documented in runtime/internal.ts while
preserving the existing stash and fallback cases.
🪄 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: 2984d606-bde1-4010-9b33-bb066edf6ecf
📒 Files selected for processing (7)
.changeset/node-adapter-direct-html-write.mdagent-feedback/dx.mdcspell.jsonpackages/run/scripts/bench-render-body.tspackages/run/src/__tests__/adapter-middleware.test.tspackages/run/src/adapter/middleware.tspackages/run/src/runtime/internal.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/run/src/runtime/internal.ts (1)
113-115: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnhandled rejection risk in
cancel()remains unfixed.
iterator.return?.(reason)'s returned promise is discarded. If the render's async iterator's.return()rejects (e.g., cleanup throwing during an aborted stream), it becomes an unhandled promise rejection, which crashes the Node process by default. This is hit whenever a client disconnects mid-render and a consumer ofresponse.body(edge adapters, or the Node middleware's fallback path ingetRender) cancels early — this was already flagged on a previous commit and is still present.🐛 Proposed fix
cancel(reason) { - iterator.return?.(reason); + return iterator.return?.(reason)?.catch(() => {}); },🤖 Prompt for 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. In `@packages/run/src/runtime/internal.ts` around lines 113 - 115, Update the cancel method to handle the promise returned by iterator.return?.(reason), ensuring any rejection is explicitly caught or otherwise consumed so cleanup failures cannot become unhandled promise rejections. Preserve cancellation behavior for iterators whose return method is absent or synchronous.
🤖 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.
Duplicate comments:
In `@packages/run/src/runtime/internal.ts`:
- Around line 113-115: Update the cancel method to handle the promise returned
by iterator.return?.(reason), ensuring any rejection is explicitly caught or
otherwise consumed so cleanup failures cannot become unhandled promise
rejections. Preserve cancellation behavior for iterators whose return method is
absent or synchronous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8521440-ece7-4fbd-9697-c62f0960638d
📒 Files selected for processing (7)
.changeset/node-adapter-direct-html-write.mdagent-feedback/dx.mdcspell.jsonpackages/run/scripts/bench-render-body.tspackages/run/src/__tests__/adapter-middleware.test.tspackages/run/src/adapter/middleware.tspackages/run/src/runtime/internal.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- cspell.json
- .changeset/node-adapter-direct-html-write.md
- agent-feedback/dx.md
…celled `toResponseBody`'s cancel discarded the promise from `iterator.return`, so a render whose cleanup rejects after a client disconnect became an unhandled rejection, crashing the process by default. cancel now awaits it and swallows the failure -- an abandoned render's cleanup error has nowhere to surface. Adds render-response tests pinning the eager iterator creation, the lazy body, the cancel path (fails against the previous code), and the `toReadable` fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UbmGqH57TZSYCfKbBtcDi
The measurement it existed for is recorded on the PR; the script does not need to ship with the repo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UbmGqH57TZSYCfKbBtcDi
2 participants
Description
The Node adapter now writes a page render's HTML strings straight to the
ServerResponse(letting Node encode UTF-8) instead of routing every response through a whatwgReadableStream/TextEncoderand reading it back out of theResponsebody.context.rendercarries the render on the response under aSymbol.forregistry key and builds the whatwg body lazily, so constructing theResponsedoesn't start consuming it.The public API is unchanged:
context.renderstill returns aResponsewhose body is a validReadableStream, so edge adapters readresponse.bodyas before. Works with Marko 5 and Marko 6.The registry symbol is deliberate. The runtime and the node adapter ship as separate esbuild bundles, so a module-scoped
Symbol()(or aWeakMap) is duplicated and never matches — which is also a fix: an earlier revision of this PR used a module-scoped symbol, so the fast path silently never engaged in the published package and only worked when running from source.Two correctness constraints come with taking the render directly. A render result is single-use, so its iterator is created eagerly — marko attaches error handling there, and with a lazy body a render that fails before anything reads it would otherwise throw uncaught, unconditionally so for HEAD requests whose body is stripped and never read. And the adapter only takes the shortcut while the response still holds the body the render was stashed with, since
clone()tees that body and drains the render.Motivation and Context
@marko/run's HTML render path is built on whatwg streams, which add fixed per-response overhead on Node. Writing the render's strings straight to the socket removes that round-trip.Local node benchmark (16 concurrent keep-alive connections, interleaved A/B against
main, median of 5 rounds): ~6% higher req/s on a 16 KB page and ~3% on a 455-byte page, with every PR pass ahead of everymainpass. That is well short of the 10–30% this PR originally claimed — those numbers predate the bundling problem above and were not reproducible here.Checklist: