Skip to content

fix(web): dispose renderToString's root synchronously, committing the response head first (#3385) - #3422

Merged
ryansolid merged 1 commit into
solidjs:nextfrom
Stephen-Psaradellis:fix/render-to-string-sync-dispose
Sep 14, 2026
Merged

ryansolid merged 1 commit into
solidjs:nextfrom
Stephen-Psaradellis:fix/render-to-string-sync-dispose

Conversation

@Stephen-Psaradellis

Copy link
Copy Markdown

Summary

Fixes #3385.

renderToString disposed its root via setTimeout, so every render in a synchronous loop (benchmarks, batch pre-rendering, Promise.all over many renders) kept its whole reactive graph alive until the next macrotask. The root is now captured and disposed in a finally before the function returns; context.trace is cleared there too.

The deferral existed so the scope-tied response primitives were not retracted before the head was read. To keep that, the request event's response stub is committed (commitResponseStub(requestEvent.response, { event })) right after assembleDocument and right before the dispose — the same head-freeze point an awaited renderToStream already uses. httpStatus/httpHeader cleanups are no-ops once the head is committed, so the declarations survive into createSSRResponse, which already passes a committed stub through. A render that throws leaves the head uncommitted: its declarations retract with the dispose, as before, and the error path can still write. No queueMicrotask was needed. This is the first of the two directions @JohannesGezachew laid out in the issue thread; his analysis of which tests a plain sync dispose breaks is what shaped it.

One behaviour change to weigh: under a request scope, renderToString now returns with event.response.committed === true, so a header write between renderToString returning and createSSRResponse hits the post-commit guard (throws in dev, logs and no-ops in prod). No test in the repo uses that window and an awaited renderToStream already behaves this way, but integrations (SolidStart) should be checked. Doc comments that named createSSRResponse as the renderToString commit point are updated, plus a @solidjs/web patch changeset.

How did you test this change?

From packages/web, server config (pnpm exec vitest run --config vite.config.server.mjs), base c66130da:

  • New test/server/render-to-string-dispose.spec.tsx (4 tests: an onCleanup counter equals N before the next synchronous render starts; a throwing render still disposes; the trace is gone when the render returns; httpStatus/httpHeader survive and reach createSSRResponse). On the unpatched base: 4 failed / 4. Patched: pass.
  • Patched, targeted: the new spec + http-components.spec.tsx + server-trace.spec.tsx — 67 passed / 67, including the three tests named in the thread (httpStatus declares status on the event's response, httpHeader declares a header, Server-Timing respects a name the app already wrote).
  • Full server suite: unpatched 850 passed / 4 failed / 2 skipped; patched 854 passed / 4 failed / 2 skipped. The 4 failures are the same in both runs and are Windows-environment ones (path separators / artifact pairing in exports-server-conditions, server-functions-adapter-request, server-functions-failure-sanitization), none touching renderToString.
  • Web client suite: 736 passed / 736. tsc --noEmit on the web build config: 0 errors. Prettier: clean.

Native compiler: the published 2.0.0-rc.8 win32 binary via SOLID_COMPILER_NATIVE (no Rust toolchain on this machine).

Disclosure. An AI system running an autonomous software project found the issue, wrote the fix and the tests, and wrote this description; I'm the account holder submitting it and I have reviewed the patch. It is free to take, change or drop.

If it's useful: I'd take scoped development work the way Solid's Open Collective ledger shows it being bought ("Development Support") — a reviewed PR with tests, invoiced as one expense on completion.

@changeset-bot

changeset-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 8ceda85

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@solidjs/web Patch
@solidjs/babel-plugin Patch
@solidjs/element Patch
@solidjs/h Patch
@solidjs/html Patch
test-integration Patch
@solidjs/compiler Patch
@solidjs/diagnostics Patch
@solidjs/signals Patch
solid-js Patch
@solidjs/universal Patch

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

@ryansolid
ryansolid merged commit 36db287 into solidjs:next Sep 14, 2026
6 checks passed
@codspeed

codspeed Bot commented Sep 14, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 17.94%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 2 improved benchmarks
✅ 158 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
color-picker: 14 colors (renderToString) 739.1 µs 579.8 µs +27.49%
search-results: 50 items (renderToString) 1.5 ms 1.4 ms +9.11%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing Stephen-Psaradellis:fix/render-to-string-sync-dispose (8ceda85) with next (7f6332a)

Open in CodSpeed

This was referenced Sep 15, 2026
This was referenced Sep 16, 2026
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.

2 participants