fix(@angular/ssr): check whether injector is destroyed #30738
Conversation
In this commit, we check whether the injector is destroyed before calling to `envInjector.get`. The application might be destroyed and `whenStable()` would resolve immediately; as thus, calling to `injector.get` is unsafe and should be guarded.
clydin
added
the
action: review
| const routerIsProvided = !!envInjector.get(ActivatedRoute, null); | ||
| const router = envInjector.get(Router); | ||
| const lastSuccessfulNavigation = router.lastSuccessfulNavigation; | ||
| const routerIsProvided = destroyed ? false : !!envInjector.get(ActivatedRoute, null); |
There was a problem hiding this comment.
What is the case when the applicationRef gets destroyed before the rendering phase? Internally in
the environment injector is also required to render the app.There was a problem hiding this comment.
I'm using autocannon for benchmarking and getting the following:
$ autocannon -c 100 -d 1 http://localhost:4200
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I can give you more information if I track down where appRef.onDestroy is being called in that situation (before the renderInternal is called).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Note: this is not happening against ng serve. This is after ng build and running node {file}.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Note the Timeout which then calls platformRef.destroy(), that's the following part of the code: asyncDestroyPlatform().
There was a problem hiding this comment.
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.
In this commit, we check whether the injector is destroyed before calling to
envInjector.get. The application might be destroyed andwhenStable()would resolve immediately; as thus, calling toinjector.getis unsafe and should be guarded.