Skip to content

Commit 322f09f

Browse files
committed
fix(@angular/ssr): destroy platform when request is aborted during rendering
Previously, aborting `request.signal` during server-side rendering caused `AngularServerApp.handle()` to reject via `promiseWithAbort`, while `renderAngular()` continued running in the background until `whenStable()` completed (or leaked the platform if a pending task never resolved). This commit forwards `request.signal` into `renderAngular()` so that aborting an in-flight request destroys the underlying `PlatformRef`, cancels any pending render timer, and exits cleanly. Additionally, `promiseWithAbort()` now attaches rejection handlers to the underlying promise even when the signal is already aborted to avoid unhandled promise rejections. Closes #34567
1 parent 7a7d184 commit 322f09f

7 files changed

Lines changed: 204 additions & 24 deletions

File tree

‎packages/angular/ssr/node/src/app-engine.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ export class AngularNodeAppEngine {
5959
* @param requestContext - Optional context for rendering, such as metadata associated with the request.
6060
* @returns A promise that resolves to the resulting HTTP response object, or `null` if no matching Angular route is found.
6161
*
62+
* @throws {DOMException} With name `'AbortError'` if the request is aborted before rendering completes.
63+
* When aborted, the underlying Angular platform is also destroyed to stop any in-progress rendering.
64+
*
6265
* @remarks A request to `https://www.example.com/page/index.html` will serve or render the Angular route
6366
* corresponding to `https://www.example.com/page`.
6467
*

‎packages/angular/ssr/src/app-engine.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,9 @@ export class AngularAppEngine {
144144
* @param requestContext - Optional context for rendering, such as metadata associated with the request.
145145
* @returns A promise that resolves to the resulting HTTP response object, or `null` if no matching Angular route is found.
146146
*
147+
* @throws {DOMException} With name `'AbortError'` if `request.signal` is aborted before rendering completes.
148+
* When aborted, the underlying Angular platform is also destroyed to stop any in-progress rendering.
149+
*
147150
* @remarks A request to `https://www.example.com/page/index.html` will serve or render the Angular route
148151
* corresponding to `https://www.example.com/page`.
149152
*

‎packages/angular/ssr/src/app.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,9 @@ export class AngularServerApp {
147147
* @param requestContext - Optional context for rendering, such as metadata associated with the request.
148148
* @returns A promise that resolves to the resulting HTTP response object, or `null` if no matching Angular route is found.
149149
*
150+
* @throws {DOMException} With name `'AbortError'` if `request.signal` is aborted before rendering completes.
151+
* When aborted, the underlying Angular platform is also destroyed to stop any in-progress rendering.
152+
*
150153
* @remarks A request to `https://www.example.com/page/index.html` will serve or render the Angular route
151154
* corresponding to `https://www.example.com/page`.
152155
*/
@@ -323,6 +326,7 @@ export class AngularServerApp {
323326
url,
324327
platformProviders,
325328
SERVER_CONTEXT_VALUE[renderMode],
329+
request.signal,
326330
);
327331

328332
if (result.hasNavigationError) {
@@ -349,13 +353,11 @@ export class AngularServerApp {
349353
controller.enqueue(this.textEncoder.encode(renderedHtml));
350354
controller.close();
351355
} catch (error) {
352-
result.destroy();
356+
await result.destroy();
353357
controller.error(error);
354358
}
355359
},
356-
cancel: () => {
357-
result.destroy();
358-
},
360+
cancel: () => result.destroy(),
359361
});
360362

361363
return new Response(stream, responseInit);

‎packages/angular/ssr/src/utils/ng.ts‎

Lines changed: 61 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,26 +47,33 @@ export type AngularBootstrap =
4747
* @param url - The application URL, used for route-based rendering in SSR.
4848
* @param platformProviders - An array of platform providers for the rendering process.
4949
* @param serverContext - A string representing the server context, providing additional metadata for SSR.
50+
* @param signal - An optional `AbortSignal` to abort the rendering process and destroy the platform.
5051
* @returns A promise resolving to an object containing:
51-
* - `hasNavigationError`: Indicates if a navigation error occurred.
52+
* - `hasNavigationError`: Indicates if a navigation error occurred or if rendering was aborted/destroyed before stability.
5253
* - `redirectTo`: (Optional) The redirect URL if a navigation redirect occurred.
53-
* - `content`: A function returning a promise that resolves to the rendered HTML string.
54+
* - `content`: (Optional) A function returning a promise that resolves to the rendered HTML string.
55+
* - `destroy`: (Optional) A function to destroy the underlying Angular platform.
5456
*/
5557
export async function renderAngular(
5658
html: string,
5759
bootstrap: AngularBootstrap,
5860
url: URL,
5961
platformProviders: StaticProvider[],
6062
serverContext: string,
63+
signal?: AbortSignal,
6164
): Promise<
6265
| { hasNavigationError: true }
6366
| {
6467
hasNavigationError: boolean;
6568
redirectTo?: string;
6669
content: () => Promise<string>;
67-
destroy: () => void;
70+
destroy: () => Promise<void>;
6871
}
6972
> {
73+
if (signal?.aborted) {
74+
return { hasNavigationError: true };
75+
}
76+
7077
// A request to `http://www.example.com/page/index.html` will render the Angular route corresponding to `http://www.example.com/page`.
7178
const urlToRender = stripIndexHtmlFromURL(url);
7279
const platformRef = platformServer([
@@ -94,14 +101,48 @@ export async function renderAngular(
94101

95102
let redirectTo: string | undefined;
96103
let hasNavigationError = true;
104+
let renderTimeoutId: ReturnType<typeof setTimeout> | undefined;
105+
let destroyPromise: Promise<void> | undefined;
106+
let rejectContent: ((reason?: unknown) => void) | undefined;
107+
108+
const destroy = (): Promise<void> => {
109+
rejectContent?.(
110+
new DOMException(
111+
`Request for: ${url} was aborted.${signal?.reason ? `\n${signal.reason}` : ''}`,
112+
'AbortError',
113+
),
114+
);
115+
rejectContent = undefined;
116+
117+
if (!destroyPromise) {
118+
clearTimeout(renderTimeoutId);
119+
signal?.removeEventListener('abort', abortHandler);
120+
destroyPromise = asyncDestroyPlatform(platformRef);
121+
}
122+
123+
return destroyPromise;
124+
};
125+
126+
const abortHandler = () => {
127+
void destroy();
128+
};
97129

98130
try {
131+
signal?.addEventListener('abort', abortHandler, { once: true });
132+
99133
let applicationRef: ApplicationRef;
100134
if (isNgModule(bootstrap)) {
101135
const moduleRef = await platformRef.bootstrapModule(bootstrap);
136+
if (platformRef.destroyed || signal?.aborted) {
137+
return { hasNavigationError: true };
138+
}
139+
102140
applicationRef = moduleRef.injector.get(ApplicationRef);
103141
} else {
104142
applicationRef = await bootstrap({ platformRef });
143+
if (platformRef.destroyed || signal?.aborted) {
144+
return { hasNavigationError: true };
145+
}
105146
}
106147

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

@@ -141,27 +182,38 @@ export async function renderAngular(
141182
}
142183

143184
return {
144-
destroy: () => void asyncDestroyPlatform(platformRef),
185+
destroy,
145186
hasNavigationError,
146187
redirectTo,
147188
content: () =>
148189
new Promise<string>((resolve, reject) => {
190+
rejectContent = reject;
191+
149192
// Defer rendering to the next event loop iteration to avoid blocking, as most operations in `renderInternal` are synchronous.
150-
setTimeout(() => {
193+
renderTimeoutId = setTimeout(() => {
194+
if (platformRef.destroyed || applicationRef.destroyed || signal?.aborted) {
195+
void destroy();
196+
197+
return;
198+
}
199+
151200
renderInternal(platformRef, applicationRef)
152201
.then(resolve)
153202
.catch(reject)
154-
.finally(() => void asyncDestroyPlatform(platformRef));
203+
.finally(() => {
204+
rejectContent = undefined;
205+
void destroy();
206+
});
155207
}, 0);
156208
}),
157209
};
158210
} catch (error) {
159-
await asyncDestroyPlatform(platformRef);
211+
await destroy();
160212

161213
throw error;
162214
} finally {
163215
if (hasNavigationError || redirectTo) {
164-
void asyncDestroyPlatform(platformRef);
216+
void destroy();
165217
}
166218
}
167219
}

‎packages/angular/ssr/src/utils/promise.ts‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
* @returns A promise that either resolves with the value of the provided `promise` or rejects with
1818
* an `AbortError` if the `AbortSignal` is triggered.
1919
*
20-
* @throws {AbortError} If the `AbortSignal` is triggered before the `promise` resolves.
20+
* @throws {DOMException} With name `'AbortError'` if the `AbortSignal` is triggered before the `promise` resolves.
2121
*/
2222
export function promiseWithAbort<T>(
2323
promise: Promise<T>,
@@ -34,12 +34,10 @@ export function promiseWithAbort<T>(
3434
// Check for abort signal
3535
if (signal.aborted) {
3636
abortHandler();
37-
38-
return;
37+
} else {
38+
signal.addEventListener('abort', abortHandler, { once: true });
3939
}
4040

41-
signal.addEventListener('abort', abortHandler, { once: true });
42-
4341
promise
4442
.then(resolve)
4543
.catch(reject)

‎packages/angular/ssr/test/app_spec.ts‎

Lines changed: 112 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,15 @@ import '@angular/compiler';
1212
/* eslint-enable import/no-unassigned-import */
1313

1414
import { APP_BASE_HREF } from '@angular/common';
15-
import { Component, PlatformRef, REQUEST, RESPONSE_INIT, inject } from '@angular/core';
15+
import {
16+
Component,
17+
DestroyRef,
18+
PendingTasks,
19+
PlatformRef,
20+
REQUEST,
21+
RESPONSE_INIT,
22+
inject,
23+
} from '@angular/core';
1624
import { ActivatedRoute, CanActivateFn, Router } from '@angular/router';
1725
import { setTimeout } from 'node:timers/promises';
1826
import { AngularServerApp } from '../src/app';
@@ -21,16 +29,21 @@ import { setAngularAppTestingManifest } from './testing-utils';
2129

2230
describe('AngularServerApp', () => {
2331
let app: AngularServerApp;
32+
let slowRouteHook: (() => void) | undefined;
2433

2534
@Component({
2635
selector: 'app-home',
2736
template: `Home works`,
2837
})
2938
class HomeComponent {
3039
constructor() {
31-
if (inject(ActivatedRoute).snapshot.data['destroyApp']) {
40+
const data = inject(ActivatedRoute).snapshot.data;
41+
if (data['destroyApp']) {
3242
inject(PlatformRef).destroy();
3343
}
44+
if (data['slow']) {
45+
slowRouteHook?.();
46+
}
3447
}
3548
}
3649

@@ -78,6 +91,7 @@ describe('AngularServerApp', () => {
7891
{ path: 'page-with-headers', component: HomeComponent },
7992
{ path: 'page-with-status', component: HomeComponent },
8093
{ path: 'page-destroy-app', component: HomeComponent, data: { destroyApp: true } },
94+
{ path: 'page-slow', component: HomeComponent, data: { slow: true } },
8195
{ path: 'redirect', redirectTo: 'home' },
8296
{ path: 'redirect-via-navigate', component: RedirectComponent },
8397
{
@@ -313,14 +327,106 @@ describe('AngularServerApp', () => {
313327
controller.abort();
314328
});
315329

330+
await expectAsync(app.handle(request)).toBeRejectedWithError(
331+
DOMException,
332+
/Request for: http:\/\/localhost\/home was aborted/,
333+
);
334+
});
335+
336+
it('should destroy the platform and stop rendering when an in-flight request is aborted', async () => {
337+
let resolveTaskReady!: () => void;
338+
const taskReady = new Promise<void>((resolve) => {
339+
resolveTaskReady = resolve;
340+
});
341+
let appDestroyed = false;
342+
let destroySpy: jasmine.Spy | undefined;
343+
344+
slowRouteHook = () => {
345+
destroySpy = spyOn(inject(PlatformRef), 'destroy').and.callThrough();
346+
const pendingTasks = inject(PendingTasks);
347+
const removeTask = pendingTasks.add();
348+
inject(DestroyRef).onDestroy(() => {
349+
appDestroyed = true;
350+
removeTask();
351+
});
352+
resolveTaskReady();
353+
};
354+
316355
try {
317-
await app.handle(request);
318-
throw new Error('Should not be called.');
319-
} catch (e) {
320-
expect(e).toBeInstanceOf(DOMException);
356+
const controller = new AbortController();
357+
const request = new Request('http://localhost/page-slow', { signal: controller.signal });
358+
359+
const handlePromise = app.handle(request);
360+
361+
// Wait until the component is instantiated and holding a pending task
362+
await taskReady;
363+
expect(appDestroyed).toBeFalse();
364+
365+
// Abort the request while `applicationRef.whenStable()` is still pending
366+
controller.abort();
367+
368+
await expectAsync(handlePromise).toBeRejectedWithError(
369+
DOMException,
370+
/Request for: http:\/\/localhost\/page-slow was aborted/,
371+
);
372+
373+
// Wait for the macrotask queue to clear since platform destruction is scheduled via setTimeout(0)
374+
await setTimeout(0);
375+
expect(destroySpy).toHaveBeenCalledTimes(1);
376+
expect(appDestroyed).toBeTrue();
377+
} finally {
378+
slowRouteHook = undefined;
321379
}
322380
});
323381

382+
it('should not bootstrap the platform if the request signal is already aborted', async () => {
383+
// Warm up the router so route extraction does not create a PlatformRef during this test
384+
const warmupResponse = await app.handle(new Request('http://localhost/home'));
385+
await warmupResponse?.text();
386+
await setTimeout(0);
387+
388+
const destroySpy = spyOn(PlatformRef.prototype, 'destroy').and.callThrough();
389+
const controller = new AbortController();
390+
controller.abort();
391+
392+
const request = new Request('http://localhost/home', { signal: controller.signal });
393+
await expectAsync(app.handle(request)).toBeRejectedWithError(DOMException);
394+
395+
await setTimeout(0);
396+
expect(destroySpy).not.toHaveBeenCalled();
397+
});
398+
399+
it('should clean up the abort event listener on request.signal after rendering completes', async () => {
400+
const controller = new AbortController();
401+
const request = new Request('http://localhost/home', { signal: controller.signal });
402+
const removeEventListenerSpy = spyOn(
403+
request.signal,
404+
'removeEventListener',
405+
).and.callThrough();
406+
407+
const response = await app.handle(request);
408+
expect(await response?.text()).toContain('Home works');
409+
await setTimeout(0);
410+
411+
expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', jasmine.any(Function));
412+
});
413+
414+
it('should reject reading response body if request is aborted after handle() resolves but before rendering completes', async () => {
415+
const controller = new AbortController();
416+
const response = await app.handle(
417+
new Request('http://localhost/home', { signal: controller.signal }),
418+
);
419+
expect(response).not.toBeNull();
420+
421+
controller.abort();
422+
423+
await expectAsync(response?.text()).toBeRejectedWithError(
424+
DOMException,
425+
/Request for: http:\/\/localhost\/home was aborted/,
426+
);
427+
await setTimeout(0);
428+
});
429+
324430
it('should return configured headers for pages with specific header settings', async () => {
325431
const response = await app.handle(new Request('http://localhost/page-with-headers'));
326432
const headers = response?.headers.entries() ?? [];

‎packages/angular/ssr/test/utils/promise_spec.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,4 +33,20 @@ describe('promiseWithAbort', () => {
3333

3434
await expectAsync(promise).toBeResolved();
3535
});
36+
37+
it('should reject with an AbortError and handle subsequent promise rejection when signal is already aborted', async () => {
38+
const abortController = new AbortController();
39+
abortController.abort('Already aborted');
40+
41+
let reject!: (reason?: unknown) => void;
42+
const underlyingPromise = new Promise<void>((_, r) => {
43+
reject = r;
44+
});
45+
const promise = promiseWithAbort(underlyingPromise, abortController.signal, 'Test operation');
46+
47+
await expectAsync(promise).toBeRejectedWithError(DOMException, /Test operation was aborted/);
48+
49+
// Rejecting the underlying promise after abort should not cause an unhandled rejection
50+
reject(new Error('Late failure'));
51+
});
3652
});

0 commit comments

Comments
 (0)