diff --git a/.changeset/fix-ssr-als-memory-leak.md b/.changeset/fix-ssr-als-memory-leak.md new file mode 100644 index 000000000000..ac3e50e75c47 --- /dev/null +++ b/.changeset/fix-ssr-als-memory-leak.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: prevent RequestEvent retention in AsyncLocalStorage async resources after SSR diff --git a/packages/kit/src/exports/internal/event.js b/packages/kit/src/exports/internal/event.js index 9d658494e1be..28ded3551aa5 100644 --- a/packages/kit/src/exports/internal/event.js +++ b/packages/kit/src/exports/internal/event.js @@ -7,7 +7,9 @@ import { IN_WEBCONTAINER } from '../../runtime/server/constants.js'; /** @type {RequestStore | null} */ let sync_store = null; -/** @type {AsyncLocalStorage | null} */ +/** @typedef {{ current: RequestStore | null }} RequestStoreContainer */ + +/** @type {AsyncLocalStorage | null} */ let als; import('node:async_hooks') @@ -63,18 +65,54 @@ export function get_request_store() { } export function try_get_request_store() { - return sync_store ?? als?.getStore() ?? null; + return sync_store ?? als?.getStore()?.current ?? null; } /** * @template T * @param {RequestStore | null} store * @param {() => T} fn + * @param {boolean} [gc_barrier] - When true, nulls the ALS container after `fn` settles so that + * async resources created during `fn` (e.g. Svelte 4 subscription callbacks) cannot retain the + * RequestStore beyond the lifetime of the call. Only needed for the SSR render context where + * long-lived subscriptions would otherwise pin the RequestEvent in memory. + * See https://github.com/nodejs/node/issues/53408 */ -export function with_request_store(store, fn) { +export function with_request_store(store, fn, gc_barrier = false) { try { sync_store = store; - return als ? als.run(store, fn) : fn(); + if (als) { + // Wrap the store in a container so that async resources created inside fn only hold a + // reference to the container object rather than the full RequestStore. When gc_barrier + // is true (SSR render path) we null container.current after fn settles, severing the + // only path from lingering async resources back to the RequestStore and allowing GC. + const container = /** @type {RequestStoreContainer} */ ({ current: store }); + const result = als.run(container, fn); + if ( + result !== null && + typeof result === 'object' && + typeof (/** @type {any} */ (result).then) === 'function' + ) { + if (gc_barrier) { + return /** @type {T} */ ( + /** @type {Promise} */ (/** @type {unknown} */ (result)).then( + (value) => { + container.current = null; + return value; + }, + (error) => { + container.current = null; + throw error; + } + ) + ); + } + return /** @type {T} */ (result); + } + if (gc_barrier) container.current = null; + return result; + } + return fn(); } finally { // Since AsyncLocalStorage is not working in webcontainers, we don't reset `sync_store` // and handle only one request at a time in `src/runtime/server/index.js`. diff --git a/packages/kit/src/exports/internal/event.spec.js b/packages/kit/src/exports/internal/event.spec.js new file mode 100644 index 000000000000..46be1ad6e233 --- /dev/null +++ b/packages/kit/src/exports/internal/event.spec.js @@ -0,0 +1,110 @@ +/** @import { RequestStore } from 'types' */ +import { describe, expect, test } from 'vitest'; +import { with_request_store, try_get_request_store, getRequestEvent } from './event.js'; + +/** @returns {RequestStore} */ +function make_store() { + return { + event: /** @type {any} */ ({ url: 'http://localhost/' }), + state: /** @type {any} */ ({}) + }; +} + +describe('with_request_store', () => { + test('exposes store via try_get_request_store during sync execution', () => { + const store = make_store(); + let seen = null; + with_request_store(store, () => { + seen = try_get_request_store(); + }); + expect(seen).toBe(store); + }); + + test('exposes store via try_get_request_store during async execution', async () => { + const store = make_store(); + let seen = null; + await with_request_store(store, async () => { + await Promise.resolve(); + seen = try_get_request_store(); + }); + expect(seen).toBe(store); + }); + + test('returns null from try_get_request_store after sync execution completes', () => { + const store = make_store(); + with_request_store(store, () => {}); + expect(try_get_request_store()).toBeNull(); + }); + + test('returns null from try_get_request_store after async execution completes', async () => { + const store = make_store(); + // Use Promise.resolve() rather than async/await to return a thenable without + // triggering the require-await lint rule on a trivial async arrow. + await with_request_store(store, () => Promise.resolve('rendered')); + expect(try_get_request_store()).toBeNull(); + }); + + // Regression test for https://github.com/sveltejs/kit/issues/15764 + // Async resources created inside als.run() inherit kResourceStore and can retain the + // RequestStore after the render completes (Node.js AsyncLocalStorage leak). The fix wraps + // the store in a container object; when gc_barrier=true (the render path), the container is + // nulled after render, allowing GC. + test('async continuations created inside render cannot access store after render completes', async () => { + const store = make_store(); + let store_seen_in_continuation = /** @type {RequestStore | null | undefined} */ (undefined); + + // Simulate what Svelte 4's component_subscribe does: create a Promise-based + // callback inside the ALS context that outlives the render. + let late_callback_promise = Promise.resolve(); // typed; overwritten inside render fn + + // gc_barrier=true mirrors the SSR render call in render.js + await with_request_store( + store, + async () => { + await Promise.resolve(); // ensure the async code path through with_request_store + late_callback_promise = new Promise((resolve) => setTimeout(resolve, 0)).then(() => { + // This runs inside the ALS context that was active when setTimeout was called + // (i.e. the context from als.run()). Without the fix, try_get_request_store() + // returns `store` here. With the fix, the container is nulled so it returns null. + store_seen_in_continuation = try_get_request_store(); + }); + }, + true + ); + + await late_callback_promise; + + expect(store_seen_in_continuation).toBeNull(); + }); + + test('getRequestEvent returns event during execution', () => { + const store = make_store(); + let event = null; + with_request_store(store, () => { + event = getRequestEvent(); + }); + expect(event).toBe(store.event); + }); + + test('getRequestEvent throws outside execution context', () => { + expect(() => getRequestEvent()).toThrowError('Can only read the current request event'); + }); + + test('nested with_request_store calls use innermost store', async () => { + const outer = make_store(); + const inner = make_store(); + let seen_in_inner = null; + let seen_after_inner = null; + + await with_request_store(outer, async () => { + await Promise.resolve(); // exercise the async path + with_request_store(inner, () => { + seen_in_inner = try_get_request_store(); + }); + seen_after_inner = try_get_request_store(); + }); + + expect(seen_in_inner).toBe(inner); + expect(seen_after_inner).toBe(outer); + }); +}); diff --git a/packages/kit/src/runtime/server/page/render.js b/packages/kit/src/runtime/server/page/render.js index 0e7c68cf6056..aed1369eeb6e 100644 --- a/packages/kit/src/runtime/server/page/render.js +++ b/packages/kit/src/runtime/server/page/render.js @@ -224,42 +224,47 @@ export async function render_response({ const state = { ...event_state, is_in_render: true }; - rendered = await with_request_store({ event, state }, async () => { - // use relative paths during rendering, so that the resulting HTML is as - // portable as possible, but reset afterwards - if (paths.relative) paths.override({ base, assets }); - - const maybe_promise = options.root.render(props, render_opts); - // We have to invoke .then eagerly here in order to kick off rendering: it's only starting on access, - // and `await maybe_promise` would eagerly access the .then property but call its function only after a tick, which is too late - // for the paths.reset() below and for any eager getRequestEvent() calls during rendering without AsyncLocalStorage available. - const rendered = - options.async && 'then' in maybe_promise - ? /** @type {ReturnType & Promise} */ ( - maybe_promise - ).then((r) => r) - : maybe_promise; - - // TODO 3.0 remove options.async - if (options.async) { - // we reset this synchronously, rather than after async rendering is complete, - // to avoid cross-talk between requests. This is a breaking change for - // anyone who opts into async SSR, since `base` and `assets` will no - // longer be relative to the current pathname. - // TODO 3.0 remove `base` and `assets` in favour of `resolve(...)` and `asset(...)` - paths.reset(); - } + rendered = await with_request_store( + { event, state }, + async () => { + // use relative paths during rendering, so that the resulting HTML is as + // portable as possible, but reset afterwards + if (paths.relative) paths.override({ base, assets }); + + const maybe_promise = options.root.render(props, render_opts); + // We have to invoke .then eagerly here in order to kick off rendering: it's only starting on access, + // and `await maybe_promise` would eagerly access the .then property but call its function only after a tick, which is too late + // for the paths.reset() below and for any eager getRequestEvent() calls during rendering without AsyncLocalStorage available. + const rendered = + options.async && 'then' in maybe_promise + ? /** @type {ReturnType & Promise} */ ( + maybe_promise + ).then((r) => r) + : maybe_promise; + + // TODO 3.0 remove options.async + if (options.async) { + // we reset this synchronously, rather than after async rendering is complete, + // to avoid cross-talk between requests. This is a breaking change for + // anyone who opts into async SSR, since `base` and `assets` will no + // longer be relative to the current pathname. + // TODO 3.0 remove `base` and `assets` in favour of `resolve(...)` and `asset(...)` + paths.reset(); + } - const { head, html, css, hashes } = /** @type {ReturnType} */ ( - options.async ? await rendered : rendered - ); + const { head, html, css, hashes } = + /** @type {ReturnType} */ ( + options.async ? await rendered : rendered + ); - if (hashes) { - csp.add_script_hashes(hashes.script); - } + if (hashes) { + csp.add_script_hashes(hashes.script); + } - return { head, html, css, hashes }; - }); + return { head, html, css, hashes }; + }, + true + ); } finally { if (DEV) { globalThis.fetch = fetch;