Skip to content

fix(@angular/ssr): check whether injector is destroyed #30738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+6 −4
Closed

fix(@angular/ssr): check whether injector is destroyed #30738

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 additions & 4 deletions packages/angular/ssr/src/utils/ng.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,15 @@ export async function renderAngular(
// Block until application is stable.
await applicationRef.whenStable();

const { destroyed } = applicationRef;

// TODO(alanagius): Find a way to avoid rendering here especially for redirects as any output will be discarded.
const envInjector = applicationRef.injector;
const routerIsProvided = !!envInjector.get(ActivatedRoute, null);
const router = envInjector.get(Router);
const lastSuccessfulNavigation = router.lastSuccessfulNavigation;
const routerIsProvided = destroyed ? false : !!envInjector.get(ActivatedRoute, null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the case when the applicationRef gets destroyed before the rendering phase? Internally in

angular-cli/packages/angular/ssr/src/utils/ng.ts

Line 131 in 7dd3bef

renderInternal(platformRef, applicationRef)
the environment injector is also required to render the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using autocannon for benchmarking and getting the following:

$ autocannon -c 100 -d 1 http://localhost:4200
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's also used in renderInternal, is that possible to avoid falling down to renderInternal if the app has been destroyed before? (i.e. throw an error prematurely)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can give you more information if I track down where appRef.onDestroy is being called in that situation (before the renderInternal is called).

Copy link
Collaborator

@alan-agius4 alan-agius4 Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We have run numerous benchmarks with autocannon and never encountered this error before. The root cause is that appRef.onDestroy is being executed too early. The proposed fix, while seemingly addressing this, is not a solution as it would prevent the page from rendering entirely.

Side note: My previous benchmarks were not run against ng serve. In my opinion, there is little value in benchmarking a development server where the application is expected to be much slower. Potentially it a race condition in the dev-server where is not geared towards handling a large set of concurrent requests and instead it is geared towards fast edit-refresh loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this is not happening against ng serve. This is after ng build and running node {file}.

Copy link
Contributor Author

@arturovt arturovt Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated ssr.mjs directly in node_modules with the following:

let stack;
applicationRef.onDestroy(() => {
    stack = new Error().stack;
});
// Block until application is stable.
await applicationRef.whenStable();
if (applicationRef.destroyed) {
    console.log('app has been destroyed prematurely: ', stack);
}
// TODO(alanagius): Find a way to avoid rendering here especially for redirects as any output will be discarded.
const envInjector = applicationRef.injector;

The stack:

app has been destroyed prematurely:  Error: 
    at file:///home/bullrun/projects/asi/esp/dist/apps/storefront/server/node_modules/@angular/ssr/fesm2022/ssr.mjs:359:21
    at file:///home/bullrun/projects/asi/esp/dist/apps/storefront/server/node_modules/@angular/core/fesm2022/debug_node-JnOYh9kg.mjs:20221:58
    at Array.forEach (<anonymous>)
    at _ApplicationRef.ngOnDestroy (file:///home/bullrun/projects/asi/esp/dist/apps/storefront/server/node_modules/@angular/core/fesm2022/debug_node-JnOYh9kg.mjs:20221:36)
    at R3Injector.destroy (file:///home/bullrun/projects/asi/esp/dist/apps/storefront/server/node_modules/@angular/core/fesm2022/root_effect_scheduler-DCy1y1b8.mjs:1878:25)
    at destroyListener (file:///home/bullrun/projects/asi/esp/dist/apps/storefront/server/node_modules/@angular/core/fesm2022/core.mjs:919:55)
    at file:///home/bullrun/projects/asi/esp/dist/apps/storefront/server/node_modules/@angular/core/fesm2022/core.mjs:1113:52
    at Set.forEach (<anonymous>)
    at _PlatformRef.destroy (file:///home/bullrun/projects/asi/esp/dist/apps/storefront/server/node_modules/@angular/core/fesm2022/core.mjs:1113:30)
    at Timeout._onTimeout (file:///home/bullrun/projects/asi/esp/dist/apps/storefront/server/node_modules/@angular/ssr/fesm2022/ssr.mjs:429:29)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the Timeout which then calls platformRef.destroy(), that's the following part of the code: asyncDestroyPlatform().

Copy link
Collaborator

Choose a reason for hiding this comment

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

The asyncDestroyPlatform function is being called before applicationRef.whenStable() resolves, which is unexpected since the call is located within the catch and finally block. Likely what is happening is that a single platformRef is being shared by two different requests. This could happen if you are using the getPlatform() or destroyPlatform() functions in your application, by any chance is this the case?

With regards to the PR itself, given all the above the changes doesn't seem right as it hides the problem, and does not resolve the root cause and hence we cannot merge it.

I strongly recommend that you file an issue with a minimal reproduction that we can take a investigate and find the root cause.

const router = destroyed ? null : envInjector.get(Router);
const lastSuccessfulNavigation = router?.lastSuccessfulNavigation;

if (!routerIsProvided) {
if (!router || !routerIsProvided) {
hasNavigationError = false;
} else if (lastSuccessfulNavigation?.finalUrl) {
hasNavigationError = false;
Expand Down