Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/angular/ssr/node/src/app-engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ export class AngularNodeAppEngine {
* @param requestContext - Optional context for rendering, such as metadata associated with the request.
* @returns A promise that resolves to the resulting HTTP response object, or `null` if no matching Angular route is found.
*
* @throws {DOMException} With name `'AbortError'` if the request is aborted before rendering completes.
* When aborted, the underlying Angular platform is also destroyed to stop any in-progress rendering.
*
* @remarks A request to `https://www.example.com/page/index.html` will serve or render the Angular route
* corresponding to `https://www.example.com/page`.
*
Expand Down
3 changes: 3 additions & 0 deletions packages/angular/ssr/src/app-engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ export class AngularAppEngine {
* @param requestContext - Optional context for rendering, such as metadata associated with the request.
* @returns A promise that resolves to the resulting HTTP response object, or `null` if no matching Angular route is found.
*
* @throws {DOMException} With name `'AbortError'` if `request.signal` is aborted before rendering completes.
* When aborted, the underlying Angular platform is also destroyed to stop any in-progress rendering.
*
* @remarks A request to `https://www.example.com/page/index.html` will serve or render the Angular route
* corresponding to `https://www.example.com/page`.
*
Expand Down
10 changes: 6 additions & 4 deletions packages/angular/ssr/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@ export class AngularServerApp {
* @param requestContext - Optional context for rendering, such as metadata associated with the request.
* @returns A promise that resolves to the resulting HTTP response object, or `null` if no matching Angular route is found.
*
* @throws {DOMException} With name `'AbortError'` if `request.signal` is aborted before rendering completes.
* When aborted, the underlying Angular platform is also destroyed to stop any in-progress rendering.
*
* @remarks A request to `https://www.example.com/page/index.html` will serve or render the Angular route
* corresponding to `https://www.example.com/page`.
*/
Expand Down Expand Up @@ -323,6 +326,7 @@ export class AngularServerApp {
url,
platformProviders,
SERVER_CONTEXT_VALUE[renderMode],
request.signal,
);

if (result.hasNavigationError) {
Expand All @@ -349,13 +353,11 @@ export class AngularServerApp {
controller.enqueue(this.textEncoder.encode(renderedHtml));
controller.close();
} catch (error) {
result.destroy();
await result.destroy();
controller.error(error);
}
},
cancel: () => {
result.destroy();
},
cancel: () => result.destroy(),
});

return new Response(stream, responseInit);
Expand Down
74 changes: 60 additions & 14 deletions packages/angular/ssr/src/utils/ng.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,26 +47,33 @@ export type AngularBootstrap =
* @param url - The application URL, used for route-based rendering in SSR.
* @param platformProviders - An array of platform providers for the rendering process.
* @param serverContext - A string representing the server context, providing additional metadata for SSR.
* @param signal - An optional `AbortSignal` to abort the rendering process and destroy the platform.
* @returns A promise resolving to an object containing:
* - `hasNavigationError`: Indicates if a navigation error occurred.
* - `hasNavigationError`: Indicates if a navigation error occurred or if rendering was aborted/destroyed before stability.
* - `redirectTo`: (Optional) The redirect URL if a navigation redirect occurred.
* - `content`: A function returning a promise that resolves to the rendered HTML string.
* - `content`: (Optional) A function returning a promise that resolves to the rendered HTML string.
* - `destroy`: (Optional) A function to destroy the underlying Angular platform.
*/
export async function renderAngular(
html: string,
bootstrap: AngularBootstrap,
url: URL,
platformProviders: StaticProvider[],
serverContext: string,
signal?: AbortSignal,
): Promise<
| { hasNavigationError: true }
| {
hasNavigationError: boolean;
redirectTo?: string;
content: () => Promise<string>;
destroy: () => void;
destroy: () => Promise<void>;
}
> {
if (signal?.aborted) {
return { hasNavigationError: true };
}

// A request to `http://www.example.com/page/index.html` will render the Angular route corresponding to `http://www.example.com/page`.
const urlToRender = stripIndexHtmlFromURL(url);
const platformRef = platformServer([
Expand Down Expand Up @@ -94,14 +101,46 @@ export async function renderAngular(

let redirectTo: string | undefined;
let hasNavigationError = true;
let renderTimeoutId: ReturnType<typeof setTimeout> | undefined;
let destroyPromise: Promise<void> | undefined;
let rejectContent: ((reason?: unknown) => void) | undefined;

const destroy = (): Promise<void> => {
rejectContent?.(
new DOMException(
`Request for: ${url} was aborted.${signal?.reason ? `\n${signal.reason}` : ''}`,
'AbortError',
),
);
rejectContent = undefined;

if (!destroyPromise) {
clearTimeout(renderTimeoutId);
signal?.removeEventListener('abort', abortHandler);
destroyPromise = asyncDestroyPlatform(platformRef);
}

return destroyPromise;
};

const abortHandler = () => void destroy();

try {
signal?.addEventListener('abort', abortHandler, { once: true });

let applicationRef: ApplicationRef;
if (isNgModule(bootstrap)) {
const moduleRef = await platformRef.bootstrapModule(bootstrap);
if (platformRef.destroyed || signal?.aborted) {
return { hasNavigationError: true };
}

applicationRef = moduleRef.injector.get(ApplicationRef);
} else {
applicationRef = await bootstrap({ platformRef });
if (platformRef.destroyed || signal?.aborted) {
return { hasNavigationError: true };
}
}

// Block until application is stable.
Expand All @@ -110,7 +149,7 @@ export async function renderAngular(
// This code protect against app destruction during bootstrapping which is a
// valid case. We should not assume the `applicationRef` is not in destroyed state.
// Calling `envInjector.get` would throw `NG0205: Injector has already been destroyed`.
if (applicationRef.destroyed) {
if (applicationRef.destroyed || signal?.aborted) {
return { hasNavigationError: true };
}

Expand All @@ -123,8 +162,6 @@ export async function renderAngular(
if (!routerIsProvided) {
hasNavigationError = false;
} else if (lastSuccessfulNavigation?.finalUrl) {
hasNavigationError = false;

const requestPrefix =
envInjector.get(APP_BASE_HREF, null, { optional: true }) ??
getFirstHeaderValue(
Expand All @@ -138,30 +175,39 @@ export async function renderAngular(
if (urlToRenderString !== finalUrl) {
redirectTo = [pathname, search, hash].join('');
}

hasNavigationError = false;
}

return {
destroy: () => void asyncDestroyPlatform(platformRef),
destroy,
hasNavigationError,
redirectTo,
content: () =>
new Promise<string>((resolve, reject) => {
rejectContent = reject;

// Defer rendering to the next event loop iteration to avoid blocking, as most operations in `renderInternal` are synchronous.
setTimeout(() => {
renderTimeoutId = setTimeout(() => {
if (platformRef.destroyed || applicationRef.destroyed || signal?.aborted) {
void destroy();

return;
}

renderInternal(platformRef, applicationRef)
.then(resolve)
.catch(reject)
.finally(() => void asyncDestroyPlatform(platformRef));
.finally(() => {
rejectContent = undefined;
void destroy();
});
}, 0);
}),
};
} catch (error) {
await asyncDestroyPlatform(platformRef);

throw error;
} finally {
if (hasNavigationError || redirectTo) {
void asyncDestroyPlatform(platformRef);
void destroy();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is there a risk this calls destroy twice across the catch and finally clauses?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can actually remove the catch block altogether since finally already handles calling destroy() when an error occurs (as hasNavigationError remains true). Done.

}
}
}
Expand Down
8 changes: 3 additions & 5 deletions packages/angular/ssr/src/utils/promise.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* @returns A promise that either resolves with the value of the provided `promise` or rejects with
* an `AbortError` if the `AbortSignal` is triggered.
*
* @throws {AbortError} If the `AbortSignal` is triggered before the `promise` resolves.
* @throws {DOMException} With name `'AbortError'` if the `AbortSignal` is triggered before the `promise` resolves.
*/
export function promiseWithAbort<T>(
promise: Promise<T>,
Expand All @@ -34,12 +34,10 @@ export function promiseWithAbort<T>(
// Check for abort signal
if (signal.aborted) {
abortHandler();

return;
} else {
signal.addEventListener('abort', abortHandler, { once: true });
Comment thread
alan-agius4 marked this conversation as resolved.
}

signal.addEventListener('abort', abortHandler, { once: true });

promise
.then(resolve)
.catch(reject)
Expand Down
118 changes: 112 additions & 6 deletions packages/angular/ssr/test/app_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,15 @@ import '@angular/compiler';
/* eslint-enable import/no-unassigned-import */

import { APP_BASE_HREF } from '@angular/common';
import { Component, PlatformRef, REQUEST, RESPONSE_INIT, inject } from '@angular/core';
import {
Component,
DestroyRef,
PendingTasks,
PlatformRef,
REQUEST,
RESPONSE_INIT,
inject,
} from '@angular/core';
import { ActivatedRoute, CanActivateFn, Router } from '@angular/router';
import { setTimeout } from 'node:timers/promises';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about node:timers/promises. 😅

import { AngularServerApp } from '../src/app';
Expand All @@ -21,16 +29,21 @@ import { setAngularAppTestingManifest } from './testing-utils';

describe('AngularServerApp', () => {
let app: AngularServerApp;
let slowRouteHook: (() => void) | undefined;

@Component({
selector: 'app-home',
template: `Home works`,
})
class HomeComponent {
constructor() {
if (inject(ActivatedRoute).snapshot.data['destroyApp']) {
const data = inject(ActivatedRoute).snapshot.data;
if (data['destroyApp']) {
inject(PlatformRef).destroy();
}
if (data['slow']) {
slowRouteHook?.();
}
}
}

Expand Down Expand Up @@ -78,6 +91,7 @@ describe('AngularServerApp', () => {
{ path: 'page-with-headers', component: HomeComponent },
{ path: 'page-with-status', component: HomeComponent },
{ path: 'page-destroy-app', component: HomeComponent, data: { destroyApp: true } },
{ path: 'page-slow', component: HomeComponent, data: { slow: true } },
{ path: 'redirect', redirectTo: 'home' },
{ path: 'redirect-via-navigate', component: RedirectComponent },
{
Expand Down Expand Up @@ -313,14 +327,106 @@ describe('AngularServerApp', () => {
controller.abort();
});

await expectAsync(app.handle(request)).toBeRejectedWithError(
DOMException,
/Request for: http:\/\/localhost\/home was aborted/,
);
});

it('should destroy the platform and stop rendering when an in-flight request is aborted', async () => {
let resolveTaskReady!: () => void;
const taskReady = new Promise<void>((resolve) => {
resolveTaskReady = resolve;
});
let appDestroyed = false;
let destroySpy: jasmine.Spy | undefined;

slowRouteHook = () => {
destroySpy = spyOn(inject(PlatformRef), 'destroy').and.callThrough();
const pendingTasks = inject(PendingTasks);
const removeTask = pendingTasks.add();
inject(DestroyRef).onDestroy(() => {
appDestroyed = true;
removeTask();
});
resolveTaskReady();
};

try {
await app.handle(request);
throw new Error('Should not be called.');
} catch (e) {
expect(e).toBeInstanceOf(DOMException);
const controller = new AbortController();
const request = new Request('http://localhost/page-slow', { signal: controller.signal });

const handlePromise = app.handle(request);

// Wait until the component is instantiated and holding a pending task
await taskReady;
expect(appDestroyed).toBeFalse();

// Abort the request while `applicationRef.whenStable()` is still pending
controller.abort();

await expectAsync(handlePromise).toBeRejectedWithError(
DOMException,
/Request for: http:\/\/localhost\/page-slow was aborted/,
);

// Wait for the macrotask queue to clear since platform destruction is scheduled via setTimeout(0)
await setTimeout(0);
expect(destroySpy).toHaveBeenCalledTimes(1);
expect(appDestroyed).toBeTrue();
} finally {
slowRouteHook = undefined;
}
});

it('should not bootstrap the platform if the request signal is already aborted', async () => {
// Warm up the router so route extraction does not create a PlatformRef during this test
const warmupResponse = await app.handle(new Request('http://localhost/home'));
await warmupResponse?.text();
await setTimeout(0);

const destroySpy = spyOn(PlatformRef.prototype, 'destroy').and.callThrough();
const controller = new AbortController();
controller.abort();

const request = new Request('http://localhost/home', { signal: controller.signal });
await expectAsync(app.handle(request)).toBeRejectedWithError(DOMException);

await setTimeout(0);
expect(destroySpy).not.toHaveBeenCalled();
});

it('should clean up the abort event listener on request.signal after rendering completes', async () => {
const controller = new AbortController();
const request = new Request('http://localhost/home', { signal: controller.signal });
const removeEventListenerSpy = spyOn(
request.signal,
'removeEventListener',
).and.callThrough();

const response = await app.handle(request);
expect(await response?.text()).toContain('Home works');
await setTimeout(0);

expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', jasmine.any(Function));
});

it('should reject reading response body if request is aborted after handle() resolves but before rendering completes', async () => {
const controller = new AbortController();
const response = await app.handle(
new Request('http://localhost/home', { signal: controller.signal }),
);
expect(response).not.toBeNull();

controller.abort();

await expectAsync(response?.text()).toBeRejectedWithError(
DOMException,
/Request for: http:\/\/localhost\/home was aborted/,
);
await setTimeout(0);
});

it('should return configured headers for pages with specific header settings', async () => {
const response = await app.handle(new Request('http://localhost/page-with-headers'));
const headers = response?.headers.entries() ?? [];
Expand Down
Loading
Loading