From fcece3658697248ab58f77b3d4a8b14d362f3c47 Mon Sep 17 00:00:00 2001 From: Arsh <69170106+lilnasy@users.noreply.github.com> Date: Mon, 18 Mar 2024 19:29:24 +0530 Subject: [PATCH] fix(rendering): allow framework renders to be cancelled (#10448) --- .changeset/slow-rabbits-care.md | 5 +++ packages/astro/src/@types/astro.ts | 4 ++ packages/astro/src/core/render-context.ts | 37 +++++++++++-------- .../src/runtime/server/render/astro/render.ts | 13 +++++-- .../src/runtime/server/render/component.ts | 17 +++++++-- .../test/fixtures/streaming/astro.config.mjs | 5 +++ .../test/fixtures/streaming/package.json | 5 ++- .../streaming/src/components/react.tsx | 8 ++++ .../streaming/src/pages/multiple-errors.astro | 7 ++++ packages/astro/test/streaming.test.js | 16 +++++--- pnpm-lock.yaml | 9 +++++ 11 files changed, 97 insertions(+), 29 deletions(-) create mode 100644 .changeset/slow-rabbits-care.md create mode 100644 packages/astro/test/fixtures/streaming/src/components/react.tsx create mode 100644 packages/astro/test/fixtures/streaming/src/pages/multiple-errors.astro diff --git a/.changeset/slow-rabbits-care.md b/.changeset/slow-rabbits-care.md new file mode 100644 index 000000000000..c7e64bd3368d --- /dev/null +++ b/.changeset/slow-rabbits-care.md @@ -0,0 +1,5 @@ +--- +"astro": patch +--- + +Fixes an issue where multiple rendering errors resulted in a crash of the SSR app server. diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index 134e464d1324..ba2588b4a0ba 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -2763,6 +2763,10 @@ export type SSRComponentMetadata = { }; export interface SSRResult { + /** + * Whether the page has failed with a non-recoverable error, or the client disconnected. + */ + cancelled: boolean; styles: Set; scripts: Set; links: Set; diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index d77ce6269d9f..7460ad751673 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -87,17 +87,16 @@ export class RenderContext { serverLike, }); const apiContext = this.createAPIContext(props); - const { type } = routeData; - const lastNext = - type === 'endpoint' - ? () => renderEndpoint(componentInstance as any, apiContext, serverLike, logger) - : type === 'redirect' - ? () => renderRedirect(this) - : type === 'page' - ? async () => { + const lastNext = async () => { + switch (routeData.type) { + case 'endpoint': return renderEndpoint(componentInstance as any, apiContext, serverLike, logger); + case 'redirect': return renderRedirect(this); + case 'page': { const result = await this.createResult(componentInstance!); - const response = await renderPage( + let response: Response; + try { + response = await renderPage( result, componentInstance?.default as any, props, @@ -105,6 +104,12 @@ export class RenderContext { streaming, routeData ); + } catch (e) { + // If there is an error in the page's frontmatter or instantiation of the RenderTemplate fails midway, + // we signal to the rest of the internals that we can ignore the results of existing renders and avoid kicking off more of them. + result.cancelled = true; + throw e; + } // Signal to the i18n middleware to maybe act on this response response.headers.set(ROUTE_TYPE_HEADER, 'page'); // Signal to the error-page-rerouting infra to let this response pass through to avoid loops @@ -112,13 +117,14 @@ export class RenderContext { response.headers.set(REROUTE_DIRECTIVE_HEADER, 'no'); } return response; - } - : type === 'fallback' - ? () => + } + case 'fallback': { + return ( new Response(null, { status: 500, headers: { [ROUTE_TYPE_HEADER]: 'fallback' } }) - : () => { - throw new Error('Unknown type of route: ' + type); - }; + ) + } + } + } const response = await callMiddleware(middleware, apiContext, lastNext); if (response.headers.get(ROUTE_TYPE_HEADER)) { @@ -200,6 +206,7 @@ export class RenderContext { // This object starts here as an empty shell (not yet the result) but then // calling the render() function will populate the object with scripts, styles, etc. const result: SSRResult = { + cancelled: false, clientDirectives, inlinedScripts, componentMetadata, diff --git a/packages/astro/src/runtime/server/render/astro/render.ts b/packages/astro/src/runtime/server/render/astro/render.ts index b8e35007b0c3..da771397edc0 100644 --- a/packages/astro/src/runtime/server/render/astro/render.ts +++ b/packages/astro/src/runtime/server/render/astro/render.ts @@ -125,6 +125,11 @@ export async function renderToReadableStream( } })(); }, + cancel() { + // If the client disconnects, + // we signal to ignore the results of existing renders and avoid kicking off more of them. + result.cancelled = true; + } }); } @@ -201,13 +206,11 @@ export async function renderToAsyncIterable( // The `next` is an object `{ promise, resolve, reject }` that we use to wait // for chunks to be pushed into the buffer. let next = promiseWithResolvers(); - // keep track of whether the client connection is still interested in the response. - let cancelled = false; const buffer: Uint8Array[] = []; // []Uint8Array const iterator: AsyncIterator = { async next() { - if (cancelled) return { done: true, value: undefined }; + if (result.cancelled) return { done: true, value: undefined }; await next.promise; @@ -243,7 +246,9 @@ export async function renderToAsyncIterable( return returnValue; }, async return() { - cancelled = true; + // If the client disconnects, + // we signal to the rest of the internals to ignore the results of existing renders and avoid kicking off more of them. + result.cancelled = true; return { done: true, value: undefined }; }, }; diff --git a/packages/astro/src/runtime/server/render/component.ts b/packages/astro/src/runtime/server/render/component.ts index 4abbfeb07987..e4cc737acce0 100644 --- a/packages/astro/src/runtime/server/render/component.ts +++ b/packages/astro/src/runtime/server/render/component.ts @@ -459,11 +459,13 @@ export async function renderComponent( slots: any = {} ): Promise { if (isPromise(Component)) { - Component = await Component; + Component = await Component + .catch(handleCancellation); } if (isFragmentComponent(Component)) { - return await renderFragmentComponent(result, slots); + return await renderFragmentComponent(result, slots) + .catch(handleCancellation); } // Ensure directives (`class:list`) are processed @@ -471,14 +473,21 @@ export async function renderComponent( // .html components if (isHTMLComponent(Component)) { - return await renderHTMLComponent(result, Component, props, slots); + return await renderHTMLComponent(result, Component, props, slots) + .catch(handleCancellation); } if (isAstroComponentFactory(Component)) { return renderAstroComponent(result, displayName, Component, props, slots); } - return await renderFrameworkComponent(result, displayName, Component, props, slots); + return await renderFrameworkComponent(result, displayName, Component, props, slots) + .catch(handleCancellation); + + function handleCancellation(e: unknown) { + if (result.cancelled) return { render() {} }; + throw e; + } } function normalizeProps(props: Record): Record { diff --git a/packages/astro/test/fixtures/streaming/astro.config.mjs b/packages/astro/test/fixtures/streaming/astro.config.mjs index e69de29bb2d1..0773be443504 100644 --- a/packages/astro/test/fixtures/streaming/astro.config.mjs +++ b/packages/astro/test/fixtures/streaming/astro.config.mjs @@ -0,0 +1,5 @@ +import react from "@astrojs/react" + +export default { + integrations: [ react() ] +} diff --git a/packages/astro/test/fixtures/streaming/package.json b/packages/astro/test/fixtures/streaming/package.json index a27a51b6dcf4..44c2626d700c 100644 --- a/packages/astro/test/fixtures/streaming/package.json +++ b/packages/astro/test/fixtures/streaming/package.json @@ -6,6 +6,9 @@ "dev": "astro dev" }, "dependencies": { - "astro": "workspace:*" + "astro": "workspace:*", + "@astrojs/react": "workspace:*", + "react": "^18.2.0", + "react-dom": "^18.2.0" } } diff --git a/packages/astro/test/fixtures/streaming/src/components/react.tsx b/packages/astro/test/fixtures/streaming/src/components/react.tsx new file mode 100644 index 000000000000..7d83e01ff29c --- /dev/null +++ b/packages/astro/test/fixtures/streaming/src/components/react.tsx @@ -0,0 +1,8 @@ +export default function ReactComp({ foo }: { foo: { bar: { baz: string[] } } }) { + return ( +
+ React Comp + {foo.bar.baz.length} +
+ ); +} diff --git a/packages/astro/test/fixtures/streaming/src/pages/multiple-errors.astro b/packages/astro/test/fixtures/streaming/src/pages/multiple-errors.astro new file mode 100644 index 000000000000..7d922ef0d809 --- /dev/null +++ b/packages/astro/test/fixtures/streaming/src/pages/multiple-errors.astro @@ -0,0 +1,7 @@ +--- +import ReactComp from '../components/react.tsx'; + +const foo = { bar: null } as any; +--- + +{foo.bar.baz.length > 0 &&
} diff --git a/packages/astro/test/streaming.test.js b/packages/astro/test/streaming.test.js index 1cfe7f42f1c1..6a0bdcf4e346 100644 --- a/packages/astro/test/streaming.test.js +++ b/packages/astro/test/streaming.test.js @@ -2,11 +2,9 @@ import assert from 'node:assert/strict'; import { after, before, describe, it } from 'node:test'; import * as cheerio from 'cheerio'; import testAdapter from './test-adapter.js'; -import { isWindows, loadFixture, streamAsyncIterator } from './test-utils.js'; +import { loadFixture, streamAsyncIterator } from './test-utils.js'; describe('Streaming', () => { - if (isWindows) return; - /** @type {import('./test-utils').Fixture} */ let fixture; @@ -79,12 +77,20 @@ describe('Streaming', () => { } assert.equal(chunks.length > 1, true); }); + + // if the offshoot promise goes unhandled, this test will pass immediately but fail the test suite + it('Stays alive on failed component renders initiated by failed render templates', async () => { + const app = await fixture.loadTestAdapterApp(); + const request = new Request('http://example.com/multiple-errors'); + const response = await app.render(request); + assert.equal(response.status, 500); + const text = await response.text(); + assert.equal(text, ''); + }); }); }); describe('Streaming disabled', () => { - if (isWindows) return; - /** @type {import('./test-utils').Fixture} */ let fixture; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8d3cdd84fff9..6ed2fefc1b53 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -3637,9 +3637,18 @@ importers: packages/astro/test/fixtures/streaming: dependencies: + '@astrojs/react': + specifier: workspace:* + version: link:../../../../integrations/react astro: specifier: workspace:* version: link:../../.. + react: + specifier: ^18.2.0 + version: 18.2.0 + react-dom: + specifier: ^18.2.0 + version: 18.2.0(react@18.2.0) packages/astro/test/fixtures/svelte-component: dependencies: