fix(rendering): allow framework renders to be cancelled (#10448)

.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.

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<SSRElement>;
 	scripts: Set<SSRElement>;
 	links: Set<SSRElement>;

packages/astro/src/core/render-context.ts

@@ -87,38 +87,44 @@ 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,
 									{},
 									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
 								if (routeData.route === '/404' || routeData.route === '/500') {
 									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,

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<void>();
-	// keep track of whether the client connection is still interested in the response.
-	let cancelled = false;
 	const buffer: Uint8Array[] = []; // []Uint8Array
 
 	const iterator: AsyncIterator<Uint8Array> = {
 		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 };
 		},
 	};

packages/astro/src/runtime/server/render/component.ts

@@ -459,26 +459,35 @@ export async function renderComponent(
 	slots: any = {}
 ): Promise<RenderInstance> {
 	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
 	props = normalizeProps(props);
 
 	// .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<string, any>): Record<string, any> {

packages/astro/test/fixtures/streaming/astro.config.mjs

@@ -0,0 +1,5 @@
+import react from "@astrojs/react"
+
+export default {
+    integrations: [ react() ]
+}

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"
   }
 }

packages/astro/test/fixtures/streaming/src/components/react.tsx

@@ -0,0 +1,8 @@
+export default function ReactComp({ foo }: { foo: { bar: { baz: string[] } } }) {
+    return (
+      <div>
+        React Comp
+        {foo.bar.baz.length}
+      </div>
+    );
+}

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;
+---
+<ReactComp foo={foo} />
+{foo.bar.baz.length > 0 && <div/>}

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;