From 8ceda85001680dfa7148a75bee7c3c325d24b342 Mon Sep 17 00:00:00 2001 From: ShortForge Date: Mon, 14 Sep 2026 01:22:53 -0500 Subject: [PATCH] fix(web): dispose renderToString's root synchronously, committing the response head first (#3385) --- .changeset/render-to-string-sync-dispose.md | 5 + packages/web/src/index.server.ts | 15 ++- packages/web/src/index.ts | 9 +- packages/web/src/server.ts | 93 +++++++++++------- .../web/test/server/http-components.spec.tsx | 4 +- .../server/render-to-string-dispose.spec.tsx | 96 +++++++++++++++++++ 6 files changed, 170 insertions(+), 52 deletions(-) create mode 100644 .changeset/render-to-string-sync-dispose.md create mode 100644 packages/web/test/server/render-to-string-dispose.spec.tsx diff --git a/.changeset/render-to-string-sync-dispose.md b/.changeset/render-to-string-sync-dispose.md new file mode 100644 index 000000000..83d2b4ebd --- /dev/null +++ b/.changeset/render-to-string-sync-dispose.md @@ -0,0 +1,5 @@ +--- +"@solidjs/web": patch +--- + +`renderToString` now disposes its reactive root synchronously before returning instead of via `setTimeout`, so a synchronous loop of renders no longer retains every graph until the next macrotask (#3385). The request event's response head is committed right before that dispose — the same head-freeze point an awaited `renderToStream` already uses — so `httpStatus`/`httpHeader` declarations still reach `createSSRResponse`. A render that throws leaves the head uncommitted and retracts its declarations as before. diff --git a/packages/web/src/index.server.ts b/packages/web/src/index.server.ts index d517d025d..a07907985 100644 --- a/packages/web/src/index.server.ts +++ b/packages/web/src/index.server.ts @@ -322,11 +322,11 @@ const headerLedgers = /* @__PURE__ */ new WeakMap>( * then recovered retracts its write instead of stomping a status a * surviving part of the tree legitimately set. Once the response head is * `committed` (head derived/sent — the shell flush of a piped - * `renderToStream`, the completion of an awaited one, `createSSRResponse` - * for a `renderToString` result), writes and retractions are no-ops. + * `renderToStream`, the completion of an awaited one or of + * `renderToString`), writes and retractions are no-ops. */ export function httpStatus(_code: number, _text?: string): void {} @@ -569,8 +569,7 @@ export function httpStatus(_code: number, _text?: string): void {} * write time and restored when the owning scope is disposed (deleted if * there was none) — a boundary that errors or recovers retracts its writes. * Once the response head is `committed` (head derived/sent — the shell - * flush of a piped `renderToStream`, the completion of an awaited one, - * `createSSRResponse` for a `renderToString` result), writes and - * retractions are no-ops. + * flush of a piped `renderToStream`, the completion of an awaited one or + * of `renderToString`), writes and retractions are no-ops. */ export function httpHeader(_name: string, _value: string, _options?: { append?: boolean }): void {} diff --git a/packages/web/src/server.ts b/packages/web/src/server.ts index 1490b84ac..47395161c 100644 --- a/packages/web/src/server.ts +++ b/packages/web/src/server.ts @@ -1562,41 +1562,59 @@ export function renderToString(code, options = {}) { registerEntryAssets(manifest); // The trace this render belongs to (see `getTraceContext`): the request's // under a request scope, the render's own otherwise — cleared with the - // render's deferred dispose so a later read outside any render does not - // find a stale one on the lingering context. + // render's dispose so a later read outside any render does not find a + // stale one on the lingering context. const context = sharedConfig.context; const requestEvent = peekRequestEvent(); context.trace = requestEvent ? traceForEvent(requestEvent) : traceFor(context, undefined); - let html = root( - d => { - setTimeout(() => { - context.trace = undefined; - d(); - }); - return resolveSSRSync(escape(code())); - }, - { id: renderId } - ); - serializeFragmentAssets("", tracking.boundaryModules, sharedConfig.context, renderId); - sharedConfig.context.noHydrate = true; - serializer.close(); - const head = renderShellHead( - headRegistry, - nonce, - null, - noScripts, - traceMetaMarkup(context.trace) - ); - return assembleDocument( - resolveSSRSelectValues(html), - tracking.emittedAssets, - tracking.preloadLinks, - tracking.inlineStyles, - scripts.length ? scripts : "", - nonce, - head, - onHead - ); + let dispose; + try { + const html = root( + d => { + dispose = d; + return resolveSSRSync(escape(code())); + }, + { id: renderId } + ); + serializeFragmentAssets("", tracking.boundaryModules, sharedConfig.context, renderId); + sharedConfig.context.noHydrate = true; + serializer.close(); + const head = renderShellHead( + headRegistry, + nonce, + null, + noScripts, + traceMetaMarkup(context.trace) + ); + const document = assembleDocument( + resolveSSRSelectValues(html), + tracking.emittedAssets, + tracking.preloadLinks, + tracking.inlineStyles, + scripts.length ? scripts : "", + nonce, + head, + onHead + ); + // Head-freeze point: the request's response head commits right before + // the render's final dispose — the same order as an awaited + // `renderToStream`'s completion — so the `httpStatus`/`httpHeader` + // declarations still live at completion survive into + // `createSSRResponse(html, event)`, which passes the committed stub + // through. A render that threw leaves the head open: its declarations + // retract with the dispose below, and the handler's error path may + // still write. + if (requestEvent && requestEvent.response) { + commitResponseStub(requestEvent.response, { event: requestEvent }); + } + return document; + } finally { + // Release the graph before returning (#3385): a deferred dispose held + // every root — and every memo under it — until the next macrotask, so + // nothing was freed across a synchronous loop of renders. + context.trace = undefined; + if (dispose) dispose(); + } } export function renderToStream( fn: () => T, @@ -5083,11 +5101,12 @@ export function createSSRResponse( * * - String results commit the stub and return a `Response` synchronously; * a `Location` on the stub becomes a real redirect - * (`getExpectedRedirectStatus`) instead of an HTML response. An awaited - * `renderToStream(...)` result arrives with its stub ALREADY committed — - * the render froze the head at completion, before its final dispose, so - * `httpStatus`/`httpHeader` declarations survive into the derived head — - * and the commit here is an idempotent pass-through for it. + * (`getExpectedRedirectStatus`) instead of an HTML response. A + * `renderToString(...)` or awaited `renderToStream(...)` result rendered + * under this event's request scope arrives with its stub ALREADY + * committed — the render froze the head at completion, before its final + * dispose, so `httpStatus`/`httpHeader` declarations survive into the + * derived head — and the commit here is an idempotent pass-through for it. * - Stream results (`renderToStream(...)`) resolve at shell flush — the * moment the head freezes: the stub is committed there (post-commit * header writes fail loudly — see `commitResponseStub`), its diff --git a/packages/web/test/server/http-components.spec.tsx b/packages/web/test/server/http-components.spec.tsx index ee52d8131..5fd253f83 100644 --- a/packages/web/test/server/http-components.spec.tsx +++ b/packages/web/test/server/http-components.spec.tsx @@ -77,8 +77,8 @@ describe("httpStatus (server primitive)", () => { storage.run(event, () => { renderToString(() => ); }); - // Read synchronously after render — renderToString defers its dispose to - // a macrotask, so the write is still in place for the integration. + // Read synchronously after render — renderToString commits the head right + // before its dispose, so the declaration survives for the integration. expect(event.response!.status).toBe(404); expect(event.response!.statusText).toBe("Not Found"); }); diff --git a/packages/web/test/server/render-to-string-dispose.spec.tsx b/packages/web/test/server/render-to-string-dispose.spec.tsx new file mode 100644 index 000000000..34a9ee829 --- /dev/null +++ b/packages/web/test/server/render-to-string-dispose.spec.tsx @@ -0,0 +1,96 @@ +/** + * @jsxImportSource @solidjs/web + * + * `renderToString` releases its reactive graph before it returns (#3385). + * The root used to be disposed via `setTimeout`, so every render in a + * synchronous loop (benchmarks, batch pre-rendering, `Promise.all` over + * many renders) was retained until the task yielded. The scope-tied + * response primitives must still survive the render: the request event's + * head freezes right before the dispose, as an awaited `renderToStream` + * already does, so `httpStatus`/`httpHeader` declarations are not retracted + * and `createSSRResponse` sees them. + */ +import { AsyncLocalStorage } from "node:async_hooks"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; +import { + createRequestEvent, + createSSRResponse, + getTraceContext, + httpHeader, + httpStatus, + renderToString +} from "@solidjs/web"; +import type { RequestEvent, ResponseStub } from "@solidjs/web"; +import { onCleanup } from "solid-js"; + +type HttpEvent = RequestEvent & { response: ResponseStub }; + +const RequestContext = Symbol.for("solid.RequestContext"); +let storage: AsyncLocalStorage; + +beforeAll(() => { + storage = new AsyncLocalStorage(); + (globalThis as any)[RequestContext] = storage; +}); + +afterAll(() => { + delete (globalThis as any)[RequestContext]; +}); + +describe("renderToString disposes its root synchronously (#3385)", () => { + test("every render's cleanup has run before the next synchronous render starts", () => { + let disposed = 0; + const Page = () => { + onCleanup(() => disposed++); + return
page
; + }; + for (let i = 0; i < 5; i++) { + expect(disposed).toBe(i); + const html = renderToString(() => ); + expect(html).toContain("page"); + expect(disposed).toBe(i + 1); + } + expect(disposed).toBe(5); + }); + + test("a render that throws still disposes what it created", () => { + let disposed = 0; + const Page = () => { + onCleanup(() => disposed++); + throw new Error("render failed"); + }; + expect(() => renderToString(() => )).toThrow("render failed"); + expect(disposed).toBe(1); + }); + + test("the render's own trace is gone as soon as the render returns", () => { + let inside: unknown; + const Reader = () => { + inside = getTraceContext(); + return t; + }; + renderToString(() => ); + expect(inside).toBeDefined(); + expect(getTraceContext()).toBeUndefined(); + }); + + test("httpStatus/httpHeader declarations survive the synchronous dispose", async () => { + const evt = createRequestEvent(new Request("https://app.example/")) as HttpEvent; + let disposed = 0; + const Page = () => { + onCleanup(() => disposed++); + httpStatus(404, "Not Found"); + httpHeader("cache-control", "no-store"); + return
not found
; + }; + const html = storage.run(evt, () => renderToString(() => )); + expect(disposed).toBe(1); + expect(evt.response.status).toBe(404); + expect(evt.response.statusText).toBe("Not Found"); + expect(evt.response.headers.get("cache-control")).toBe("no-store"); + const response = createSSRResponse(html, evt); + expect(response.status).toBe(404); + expect(response.headers.get("cache-control")).toBe("no-store"); + expect(await response.text()).toContain("not found"); + }); +});