diff --git a/.changeset/great-hoops-tickle.md b/.changeset/great-hoops-tickle.md new file mode 100644 index 0000000000..94b32588f1 --- /dev/null +++ b/.changeset/great-hoops-tickle.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: run `onDestroy` callbacks when a server render throws diff --git a/packages/svelte/src/internal/server/abort-signal.js b/packages/svelte/src/internal/server/abort-signal.js index a769a46e3d..16047bdbb1 100644 --- a/packages/svelte/src/internal/server/abort-signal.js +++ b/packages/svelte/src/internal/server/abort-signal.js @@ -1,13 +1,12 @@ -import { STALE_REACTION } from '#client/constants'; +import { ssr_context } from './context.js'; -/** @type {AbortController | null} */ -let controller = null; +export function getAbortSignal() { + let context = ssr_context; -export function abort() { - controller?.abort(STALE_REACTION); - controller = null; -} + while (context !== null) { + if (context.r !== null) return context.r.global.get_abort_signal(); + context = context.p; + } -export function getAbortSignal() { - return (controller ??= new AbortController()).signal; + return new AbortController().signal; } diff --git a/packages/svelte/src/internal/server/renderer.js b/packages/svelte/src/internal/server/renderer.js index 5fb0bb86d5..1a5199d18e 100644 --- a/packages/svelte/src/internal/server/renderer.js +++ b/packages/svelte/src/internal/server/renderer.js @@ -3,7 +3,7 @@ /** @import { Csp, RenderOutput, SyncRenderOutput, Sha256Source } from '../../server/public.js' */ /** @import { MaybePromise } from '#shared' */ import { async_mode_flag } from '../flags/index.js'; -import { abort } from './abort-signal.js'; +import { STALE_REACTION } from '../client/constants.js'; import { pop, push, set_ssr_context, ssr_context } from './context.js'; import * as e from './errors.js'; import * as w from './warnings.js'; @@ -180,7 +180,7 @@ export class Renderer { // prevent unhandled rejections, and attach the promise to the renderer instance // so that rejections correctly cause rendering to fail promise.catch(noop); - this.promise = promise; + this.promise = this.global.track(promise); return promises; } @@ -225,7 +225,7 @@ export class Renderer { e.await_invalid(); } - child.promise = result; + child.promise = child.global.track(result); } return child; @@ -273,7 +273,7 @@ export class Renderer { e.await_invalid(); } result.catch(noop); - child.promise = result; + child.promise = child.global.track(result); } } catch (error) { // synchronous errors are handled here, async errors will be handled in #collect_content_async @@ -293,12 +293,14 @@ export class Renderer { e.await_invalid(); } - child.promise = /** @type {Promise} */ (result).then((transformed) => { - set_ssr_context(parent_context); - child.#out.push(Renderer.#serialize_failed_boundary(transformed)); - failed_snippet(child, transformed, noop); - child.#out.push(BLOCK_CLOSE); - }); + child.promise = child.global.track( + /** @type {Promise} */ (result).then((transformed) => { + set_ssr_context(parent_context); + child.#out.push(Renderer.#serialize_failed_boundary(transformed)); + failed_snippet(child, transformed, noop); + child.#out.push(BLOCK_CLOSE); + }) + ); child.promise.catch(noop); } else { child.#out.push(Renderer.#serialize_failed_boundary(result)); @@ -317,8 +319,11 @@ export class Renderer { */ component(fn, component_fn) { push(component_fn); - const child = this.child(fn); - child.#is_component_body = true; + // mark before running so `onDestroy` callbacks are still collected if `fn` throws + this.child((renderer) => { + renderer.#is_component_body = true; + return fn(renderer); + }); pop(); } @@ -626,6 +631,49 @@ export class Renderer { } } + /** + * Runs every `onDestroy` callback in this renderer tree. On a failed render, + * cleanup errors are suppressed so they do not mask the render error. + * @param {boolean} suppress_errors + */ + #run_on_destroy(suppress_errors) { + let first_error; + let has_error = false; + + for (const cleanup of this.#collect_on_destroy()) { + try { + cleanup(); + } catch (error) { + if (!suppress_errors && !has_error) { + first_error = error; + has_error = true; + } + } + } + + if (has_error) throw first_error; + } + + /** + * @param {'sync' | 'async'} mode + * @param {{ idPrefix?: string; csp?: Csp; transformError?: (error: unknown) => unknown }} options + * @returns {Renderer} + */ + static #create(mode, options) { + if (options.idPrefix?.includes('--')) { + e.invalid_id_prefix(); + } + + return new Renderer( + new SSRState( + mode, + options.idPrefix ? options.idPrefix + '-' : '', + options.csp, + options.transformError + ) + ); + } + /** * Render a component. Throws if any of the children are performing asynchronous work. * @@ -636,13 +684,27 @@ export class Renderer { */ static #render(component, options) { var previous_context = ssr_context; + const renderer = Renderer.#create('sync', options); + /** @type {AccumulatedContent | undefined} */ + let result; + let render_error; + let failed = false; + try { - const renderer = Renderer.#open_render('sync', component, options); + try { + Renderer.#open_render(renderer, component, options); + result = Renderer.#close_render(renderer.#collect_content(), renderer); + } catch (error) { + render_error = error; + failed = true; + } + + renderer.#run_on_destroy(failed); + if (failed) throw render_error; - const content = renderer.#collect_content(); - return Renderer.#close_render(content, renderer); + return /** @type {AccumulatedContent} */ (result); } finally { - abort(); + renderer.global.abort(); set_ssr_context(previous_context); } } @@ -657,18 +719,35 @@ export class Renderer { */ static async #render_async(component, options) { const previous_context = ssr_context; + const renderer = Renderer.#create('async', options); + /** @type {(AccumulatedContent & { hashes: { script: Sha256Source[] } }) | undefined} */ + let result; + let render_error; + let failed = false; try { - const renderer = Renderer.#open_render('async', component, options); - const content = await renderer.#collect_content_async(); - const hydratables = await renderer.#collect_hydratables(); - if (hydratables !== null) { - content.head = hydratables + content.head; + try { + Renderer.#open_render(renderer, component, options); + const content = await renderer.#collect_content_async(); + const hydratables = await renderer.#collect_hydratables(); + if (hydratables !== null) { + content.head = hydratables + content.head; + } + result = Renderer.#close_render(content, renderer); + } catch (error) { + render_error = error; + failed = true; + renderer.global.abort(); + await renderer.global.settle(); } - return Renderer.#close_render(content, renderer); + + renderer.#run_on_destroy(failed); + if (failed) throw render_error; + + return /** @type {AccumulatedContent & { hashes: { script: Sha256Source[] } }} */ (result); } finally { set_ssr_context(previous_context); - abort(); + renderer.global.abort(); } } @@ -760,28 +839,15 @@ export class Renderer { /** * @template {Record} Props - * @param {'sync' | 'async'} mode + * @param {Renderer} renderer * @param {import('svelte').Component} component * @param {{ props?: Omit; context?: Map; idPrefix?: string; csp?: Csp; transformError?: (error: unknown) => unknown }} options - * @returns {Renderer} + * @returns {void} */ - static #open_render(mode, component, options) { - if (options.idPrefix?.includes('--')) { - e.invalid_id_prefix(); - } - + static #open_render(renderer, component, options) { var previous_context = ssr_context; try { - const renderer = new Renderer( - new SSRState( - mode, - options.idPrefix ? options.idPrefix + '-' : '', - options.csp, - options.transformError - ) - ); - /** @type {SSRContext} */ const context = { p: null, c: options.context ?? null, r: renderer }; set_ssr_context(context); @@ -790,8 +856,6 @@ export class Renderer { // @ts-expect-error component(renderer, options.props ?? {}); renderer.push(BLOCK_CLOSE); - - return renderer; } finally { set_ssr_context(previous_context); } @@ -803,10 +867,6 @@ export class Renderer { * @returns {AccumulatedContent & { hashes: { script: Sha256Source[] } }} */ static #close_render(content, renderer) { - for (const cleanup of renderer.#collect_on_destroy()) { - cleanup(); - } - let head = content.head + renderer.global.get_title(); let body = content.body; @@ -890,6 +950,14 @@ export class SSRState { /** @readonly @type {Set<{ hash: string; code: string }>} */ css = new Set(); + /** @type {Set>} */ + #pending = new Set(); + + /** @type {AbortController | null} */ + #controller = null; + + #aborted = false; + /** * `transformError` passed to `render`. Called when an error boundary catches an error. * Throws by default if unset in `render`. @@ -920,6 +988,38 @@ export class SSRState { this.uid = () => `${id_prefix}s${uid++}`; } + /** + * @template T + * @param {Promise} promise + * @returns {Promise} + */ + track(promise) { + this.#pending.add(promise); + promise.then( + () => this.#pending.delete(promise), + () => this.#pending.delete(promise) + ); + return promise; + } + + async settle() { + while (this.#pending.size > 0) { + await Promise.allSettled([...this.#pending]); + } + } + + abort() { + if (this.#aborted) return; + this.#aborted = true; + this.#controller?.abort(STALE_REACTION); + } + + get_abort_signal() { + const controller = (this.#controller ??= new AbortController()); + if (this.#aborted) controller.abort(STALE_REACTION); + return controller.signal; + } + get_title() { return this.#title.value; } diff --git a/packages/svelte/src/internal/server/renderer.test.ts b/packages/svelte/src/internal/server/renderer.test.ts index 8e98c41796..1adfdda64c 100644 --- a/packages/svelte/src/internal/server/renderer.test.ts +++ b/packages/svelte/src/internal/server/renderer.test.ts @@ -2,6 +2,7 @@ import { afterAll, beforeAll, describe, expect, test } from 'vitest'; import { Renderer, SSRState } from './renderer.js'; import type { Component } from 'svelte'; import { disable_async_mode_flag, enable_async_mode_flag } from '../flags/index.js'; +import { getAbortSignal } from './abort-signal.js'; test('collects synchronous body content by default', () => { const component = (renderer: Renderer) => { @@ -466,4 +467,176 @@ describe('async', () => { await Renderer.render(component as unknown as Component); expect(destroyed).toEqual(['c', 'e', 'a', 'b', 'b*', 'd']); }); + + test('on_destroy callbacks run when a sync render throws', () => { + const destroyed: string[] = []; + const component = (renderer: Renderer) => { + renderer.component((renderer) => { + renderer.on_destroy(() => destroyed.push('a')); + renderer.child(() => { + throw new Error('boom'); + }); + }); + }; + + expect(() => Renderer.render(component as unknown as Component).body).toThrow('boom'); + expect(destroyed).toEqual(['a']); + }); + + test('on_destroy callbacks run when an async render rejects', async () => { + const destroyed: string[] = []; + const component = (renderer: Renderer) => { + renderer.component((renderer) => { + renderer.on_destroy(() => destroyed.push('a')); + renderer.child(async () => { + await Promise.resolve(); + throw new Error('boom'); + }); + }); + }; + + await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom'); + expect(destroyed).toEqual(['a']); + }); + + test('on_destroy waits for in-flight renderers when an async render rejects', async () => { + const events: string[] = []; + let initialised = false; + + const component = (renderer: Renderer) => { + renderer.component((renderer) => { + // rejects while the sibling component below is still in flight + renderer.child(async () => { + await Promise.resolve(); + throw new Error('boom'); + }); + + renderer.component((renderer) => { + renderer.on_destroy(() => events.push(`before-await (initialised: ${initialised})`)); + renderer.child(async () => { + await new Promise((f) => setTimeout(f, 10)); + initialised = true; + renderer.on_destroy(() => events.push('after-await')); + }); + }); + }); + }; + + await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom'); + expect(events).toEqual(['before-await (initialised: true)', 'after-await']); + }); + + test('aborts in-flight renderers before waiting for them', async () => { + const events: string[] = []; + const component = (renderer: Renderer) => { + renderer.component((renderer) => { + renderer.child(async () => { + await Promise.resolve(); + throw new Error('boom'); + }); + + renderer.component((renderer) => { + renderer.on_destroy(() => events.push('destroyed')); + renderer.child(async () => { + const signal = getAbortSignal(); + await new Promise((_, reject) => { + signal.addEventListener('abort', () => reject(signal.reason), { once: true }); + }); + }); + }); + }); + }; + + await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom'); + expect(events).toEqual(['destroyed']); + }); + + test('on_destroy waits for every run invocation when an async render rejects', async () => { + const events: string[] = []; + let initialised = false; + const component = (renderer: Renderer) => { + renderer.component((renderer) => { + renderer.on_destroy(() => events.push(`destroyed (initialised: ${initialised})`)); + renderer.run([ + async () => { + await new Promise((resolve) => setTimeout(resolve, 10)); + initialised = true; + renderer.on_destroy(() => events.push('destroyed after await')); + } + ]); + renderer.run([ + async () => { + await Promise.resolve(); + throw new Error('boom'); + } + ]); + }); + }; + + await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom'); + expect(events).toEqual(['destroyed (initialised: true)', 'destroyed after await']); + }); + + test('abort signals are scoped to a render', async () => { + let signal: AbortSignal; + let start!: () => void; + let resume!: () => void; + const started = new Promise((resolve) => (start = resolve)); + const resumed = new Promise((resolve) => (resume = resolve)); + const component = (renderer: Renderer) => { + renderer.child(async () => { + signal = getAbortSignal(); + start(); + await resumed; + }); + }; + + const first_render = Promise.resolve(Renderer.render(component as unknown as Component)); + await started; + await Renderer.render((() => {}) as unknown as Component); + expect(signal!.aborted).toBe(false); + + resume(); + await first_render; + expect(signal!.aborted).toBe(true); + }); + + test('a throwing on_destroy callback does not mask a sync render error', () => { + const destroyed: string[] = []; + const component = (renderer: Renderer) => { + renderer.component((renderer) => { + renderer.on_destroy(() => { + destroyed.push('a'); + throw new Error('cleanup failed'); + }); + renderer.on_destroy(() => destroyed.push('b')); + renderer.child(() => { + throw new Error('boom'); + }); + }); + }; + + expect(() => Renderer.render(component as unknown as Component).body).toThrow('boom'); + expect(destroyed).toEqual(['a', 'b']); + }); + + test('a throwing on_destroy callback does not mask an async render error', async () => { + const destroyed: string[] = []; + const component = (renderer: Renderer) => { + renderer.component((renderer) => { + renderer.on_destroy(() => { + destroyed.push('a'); + throw new Error('cleanup failed'); + }); + renderer.on_destroy(() => destroyed.push('b')); + renderer.child(async () => { + await Promise.resolve(); + throw new Error('boom'); + }); + }); + }; + + await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom'); + expect(destroyed).toEqual(['a', 'b']); + }); });