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/renderer.js b/packages/svelte/src/internal/server/renderer.js index 35aac64721..0ad03c63fc 100644 --- a/packages/svelte/src/internal/server/renderer.js +++ b/packages/svelte/src/internal/server/renderer.js @@ -44,6 +44,12 @@ export class Renderer { */ #on_destroy = undefined; + /** + * Whether the `onDestroy` callbacks of this renderer tree have been run. + * @type {boolean} + */ + #destroyed = false; + /** * Whether this renderer is a component body. * @type {boolean} @@ -316,8 +322,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(); } @@ -624,6 +633,75 @@ export class Renderer { } } + /** + * Runs the `onDestroy` callbacks of this renderer tree at most once, + * whether the render succeeded or failed. + */ + #run_on_destroy() { + if (this.#destroyed) return; + this.#destroyed = true; + + for (const cleanup of this.#collect_on_destroy()) { + cleanup(); + } + } + + /** + * Waits until every promise in the tree has settled, including promises created + * while waiting. This makes `#collect_on_destroy` safe to call after a failed + * async render, where siblings of the rejected renderer are still in flight. + */ + async #settle() { + /** @type {Set>} */ + const seen = new Set(); + + /** @type {Promise[]} */ + let pending; + + do { + pending = []; + this.#collect_pending(seen, pending); + await Promise.allSettled(pending); + } while (pending.length > 0); + } + + /** + * @param {Set>} seen + * @param {Promise[]} pending + */ + #collect_pending(seen, pending) { + if (this.promise !== undefined && !seen.has(this.promise)) { + seen.add(this.promise); + pending.push(this.promise); + } + + for (const child of this.#out) { + if (typeof child !== 'string') { + child.#collect_pending(seen, pending); + } + } + } + + /** + * @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. * @@ -634,11 +712,23 @@ export class Renderer { */ static #render(component, options) { var previous_context = ssr_context; + const renderer = Renderer.#create('sync', options); try { - const renderer = Renderer.#open_render('sync', component, options); + Renderer.#open_render(renderer, component, options); const content = renderer.#collect_content(); - return Renderer.#close_render(content, renderer); + const result = Renderer.#close_render(content, renderer); + + renderer.#run_on_destroy(); + return result; + } catch (error) { + try { + renderer.#run_on_destroy(); + } catch { + // a throwing cleanup must not mask the error that failed the render + } + + throw error; } finally { abort(); set_ssr_context(previous_context); @@ -655,15 +745,31 @@ export class Renderer { */ static async #render_async(component, options) { const previous_context = ssr_context; + const renderer = Renderer.#create('async', options); try { - const renderer = Renderer.#open_render('async', component, options); + 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; } - return Renderer.#close_render(content, renderer); + const result = Renderer.#close_render(content, renderer); + + renderer.#run_on_destroy(); + return result; + } catch (error) { + // in-flight siblings of the rejected renderer must finish initialising + // before their cleanup runs, and may register more callbacks after resuming + await renderer.#settle(); + + try { + renderer.#run_on_destroy(); + } catch { + // a throwing cleanup must not mask the error that failed the render + } + + throw error; } finally { set_ssr_context(previous_context); abort(); @@ -758,28 +864,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); @@ -788,8 +881,6 @@ export class Renderer { // @ts-expect-error component(renderer, options.props ?? {}); renderer.push(BLOCK_CLOSE); - - return renderer; } finally { set_ssr_context(previous_context); } @@ -801,10 +892,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; diff --git a/packages/svelte/src/internal/server/renderer.test.ts b/packages/svelte/src/internal/server/renderer.test.ts index 8e98c41796..e67a47f107 100644 --- a/packages/svelte/src/internal/server/renderer.test.ts +++ b/packages/svelte/src/internal/server/renderer.test.ts @@ -466,4 +466,93 @@ 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('a throwing on_destroy callback does not mask a sync render error', () => { + const component = (renderer: Renderer) => { + renderer.component((renderer) => { + renderer.on_destroy(() => { + throw new Error('cleanup failed'); + }); + renderer.child(() => { + throw new Error('boom'); + }); + }); + }; + + expect(() => Renderer.render(component as unknown as Component).body).toThrow('boom'); + }); + + test('a throwing on_destroy callback does not mask an async render error', async () => { + const component = (renderer: Renderer) => { + renderer.component((renderer) => { + renderer.on_destroy(() => { + throw new Error('cleanup failed'); + }); + renderer.child(async () => { + await Promise.resolve(); + throw new Error('boom'); + }); + }); + }; + + await expect(Renderer.render(component as unknown as Component)).rejects.toThrow('boom'); + }); });