From 0d957c29ff119df2734355779ab89431cd98defb Mon Sep 17 00:00:00 2001 From: Krzysztof Platis Date: Tue, 8 Oct 2024 00:19:15 +0200 Subject: [PATCH] fix(platform-server): destroy `PlatformRef` when error happens during the `bootstrap()` phase The `bootstrap()` phase might fail e.g. due to an rejected promise in some `APP_INIIALIZER`. If `PlatformRef` is not destroyed, then the main app's injector is not destroyed and therefore `ngOnDestroy` hooks of singleton services is not called on the end (failure) of SSR. This could lead to possible memory leaks in custom SSR apps, if their singleton services' `ngOnDestroy` hooks contained an important teardown logic (e.g. unsubscribing from RxJS observable). Note: I needed to fix by the way another thing too: now we destroy `moduleRef` when `platformInjector` is destroyed - by setting a `PLATFORM_DESTROY_LISTENER` fixes #58111 --- packages/core/src/platform/bootstrap.ts | 20 ++- packages/core/src/platform/platform_ref.ts | 6 +- packages/platform-server/src/utils.ts | 49 +++--- .../platform-server/test/integration_spec.ts | 150 ++++++++++++++++++ 4 files changed, 198 insertions(+), 27 deletions(-) diff --git a/packages/core/src/platform/bootstrap.ts b/packages/core/src/platform/bootstrap.ts index 88ef785f2306..c9d802754770 100644 --- a/packages/core/src/platform/bootstrap.ts +++ b/packages/core/src/platform/bootstrap.ts @@ -26,21 +26,24 @@ import {Injector} from '../di'; import {InternalNgModuleRef, NgModuleRef} from '../linker/ng_module_factory'; import {stringify} from '../util/stringify'; -export interface ModuleBootstrapConfig { +export interface BootstrapConfig { + platformInjector: Injector; +} + +export interface ModuleBootstrapConfig extends BootstrapConfig { moduleRef: InternalNgModuleRef; allPlatformModules: NgModuleRef[]; } -export interface ApplicationBootstrapConfig { +export interface ApplicationBootstrapConfig extends BootstrapConfig { r3Injector: R3Injector; - platformInjector: Injector; rootComponent: Type | undefined; } function isApplicationBootstrapConfig( config: ApplicationBootstrapConfig | ModuleBootstrapConfig, ): config is ApplicationBootstrapConfig { - return !!(config as ApplicationBootstrapConfig).platformInjector; + return !(config as ModuleBootstrapConfig).moduleRef; } export function bootstrap( @@ -91,9 +94,9 @@ export function bootstrap( }); }); + // If the whole platform is destroyed, invoke the `destroy` method + // for all bootstrapped applications as well. if (isApplicationBootstrapConfig(config)) { - // If the whole platform is destroyed, invoke the `destroy` method - // for all bootstrapped applications as well. const destroyListener = () => envInjector.destroy(); const onPlatformDestroyListeners = config.platformInjector.get(PLATFORM_DESTROY_LISTENERS); onPlatformDestroyListeners.add(destroyListener); @@ -103,9 +106,14 @@ export function bootstrap( onPlatformDestroyListeners.delete(destroyListener); }); } else { + const destroyListener = () => config.moduleRef.destroy(); + const onPlatformDestroyListeners = config.platformInjector.get(PLATFORM_DESTROY_LISTENERS); + onPlatformDestroyListeners.add(destroyListener); + config.moduleRef.onDestroy(() => { remove(config.allPlatformModules, config.moduleRef); onErrorSubscription.unsubscribe(); + onPlatformDestroyListeners.delete(destroyListener); }); } diff --git a/packages/core/src/platform/platform_ref.ts b/packages/core/src/platform/platform_ref.ts index 600d00cf84c8..69d4eeef2035 100644 --- a/packages/core/src/platform/platform_ref.ts +++ b/packages/core/src/platform/platform_ref.ts @@ -79,7 +79,11 @@ export class PlatformRef { allAppProviders, ); - return bootstrap({moduleRef, allPlatformModules: this._modules}); + return bootstrap({ + moduleRef, + allPlatformModules: this._modules, + platformInjector: this.injector, + }); } /** diff --git a/packages/platform-server/src/utils.ts b/packages/platform-server/src/utils.ts index 060ecdaf4b3d..9b6642c2eb88 100644 --- a/packages/platform-server/src/utils.ts +++ b/packages/platform-server/src/utils.ts @@ -210,18 +210,21 @@ async function _render(platformRef: PlatformRef, applicationRef: ApplicationRef) } appendServerContextInfo(applicationRef); - const output = platformState.renderToString(); - // Destroy the application in a macrotask, this allows pending promises to be settled and errors - // to be surfaced to the users. - await new Promise((resolve) => { + return platformState.renderToString(); +} + +/** + * Destroy the application in a macrotask, this allows pending promises to be settled and errors + * to be surfaced to the users. + */ +function asyncDestroyPlatform(platformRef: PlatformRef): Promise { + return new Promise((resolve) => { setTimeout(() => { platformRef.destroy(); resolve(); }, 0); }); - - return output; } /** @@ -264,9 +267,13 @@ export async function renderModule( ): Promise { const {document, url, extraProviders: platformProviders} = options; const platformRef = createServerPlatform({document, url, platformProviders}); - const moduleRef = await platformRef.bootstrapModule(moduleType); - const applicationRef = moduleRef.injector.get(ApplicationRef); - return _render(platformRef, applicationRef); + try { + const moduleRef = await platformRef.bootstrapModule(moduleType); + const applicationRef = moduleRef.injector.get(ApplicationRef); + return await _render(platformRef, applicationRef); + } finally { + await asyncDestroyPlatform(platformRef); + } } /** @@ -299,15 +306,17 @@ export async function renderApplication( startMeasuring(renderAppLabel); const platformRef = createServerPlatform(options); - - startMeasuring(bootstrapLabel); - const applicationRef = await bootstrap(); - stopMeasuring(bootstrapLabel); - - startMeasuring(_renderLabel); - const rendered = await _render(platformRef, applicationRef); - stopMeasuring(_renderLabel); - - stopMeasuring(renderAppLabel); - return rendered; + try { + startMeasuring(bootstrapLabel); + const applicationRef = await bootstrap(); + stopMeasuring(bootstrapLabel); + + startMeasuring(_renderLabel); + const rendered = await _render(platformRef, applicationRef); + stopMeasuring(_renderLabel); + return rendered; + } finally { + await asyncDestroyPlatform(platformRef); + stopMeasuring(renderAppLabel); + } } diff --git a/packages/platform-server/test/integration_spec.ts b/packages/platform-server/test/integration_spec.ts index dcfead07f56d..b2facece402c 100644 --- a/packages/platform-server/test/integration_spec.ts +++ b/packages/platform-server/test/integration_spec.ts @@ -42,6 +42,9 @@ import { ViewEncapsulation, ɵPendingTasks as PendingTasks, ɵwhenStable as whenStable, + APP_INITIALIZER, + inject, + getPlatform, } from '@angular/core'; import {SSR_CONTENT_INTEGRITY_MARKER} from '@angular/core/src/hydration/utils'; import {TestBed} from '@angular/core/testing'; @@ -1076,6 +1079,153 @@ class HiddenModule {} ); }, ); + + it( + `should call onOnDestroy of a service after a successful render` + + `(standalone: ${isStandalone})`, + async () => { + let wasServiceNgOnDestroyCalled = false; + + @Injectable({providedIn: 'root'}) + class DestroyableService { + ngOnDestroy() { + wasServiceNgOnDestroyCalled = true; + } + } + + const SuccessfulAppInitializerProviders = [ + { + provide: APP_INITIALIZER, + useFactory: () => { + inject(DestroyableService); + return () => Promise.resolve(); // Success in APP_INITIALIZER + }, + multi: true, + }, + ]; + + @NgModule({ + providers: SuccessfulAppInitializerProviders, + imports: [MyServerAppModule, ServerModule], + bootstrap: [MyServerApp], + }) + class ServerSuccessfulAppInitializerModule {} + + const ServerSuccessfulAppInitializerAppStandalone = getStandaloneBootstrapFn( + createMyServerApp(true), + SuccessfulAppInitializerProviders, + ); + + const options = {document: doc}; + const bootstrap = isStandalone + ? renderApplication(ServerSuccessfulAppInitializerAppStandalone, options) + : renderModule(ServerSuccessfulAppInitializerModule, options); + await bootstrap; + + expect(getPlatform()).withContext('PlatformRef should be destroyed').toBeNull(); + expect(wasServiceNgOnDestroyCalled) + .withContext('DestroyableService.ngOnDestroy() should be called') + .toBeTrue(); + }, + ); + + it( + `should call onOnDestroy of a service after some APP_INITIALIZER fails ` + + `(standalone: ${isStandalone})`, + async () => { + let wasServiceNgOnDestroyCalled = false; + + @Injectable({providedIn: 'root'}) + class DestroyableService { + ngOnDestroy() { + wasServiceNgOnDestroyCalled = true; + } + } + + const FailingAppInitializerProviders = [ + { + provide: APP_INITIALIZER, + useFactory: () => { + inject(DestroyableService); + return () => Promise.reject('Error in APP_INITIALIZER'); + }, + multi: true, + }, + ]; + + @NgModule({ + providers: FailingAppInitializerProviders, + imports: [MyServerAppModule, ServerModule], + bootstrap: [MyServerApp], + }) + class ServerFailingAppInitializerModule {} + + const ServerFailingAppInitializerAppStandalone = getStandaloneBootstrapFn( + createMyServerApp(true), + FailingAppInitializerProviders, + ); + + const options = {document: doc}; + const bootstrap = isStandalone + ? renderApplication(ServerFailingAppInitializerAppStandalone, options) + : renderModule(ServerFailingAppInitializerModule, options); + await expectAsync(bootstrap).toBeRejectedWith('Error in APP_INITIALIZER'); + + expect(getPlatform()).withContext('PlatformRef should be destroyed').toBeNull(); + expect(wasServiceNgOnDestroyCalled) + .withContext('DestroyableService.ngOnDestroy() should be called') + .toBeTrue(); + }, + ); + + it( + `should call onOnDestroy of a service after an error happens in a root component's constructor ` + + `(standalone: ${isStandalone})`, + async () => { + let wasServiceNgOnDestroyCalled = false; + + @Injectable({providedIn: 'root'}) + class DestroyableService { + ngOnDestroy() { + wasServiceNgOnDestroyCalled = true; + } + } + + @Component({ + standalone: isStandalone, + selector: 'app', + template: `Works!`, + }) + class MyServerFailingConstructorApp { + constructor() { + inject(DestroyableService); + throw 'Error in constructor of the root component'; + } + } + + @NgModule({ + declarations: [MyServerFailingConstructorApp], + imports: [MyServerAppModule, ServerModule], + bootstrap: [MyServerFailingConstructorApp], + }) + class MyServerFailingConstructorAppModule {} + + const MyServerFailingConstructorAppStandalone = getStandaloneBootstrapFn( + MyServerFailingConstructorApp, + ); + const options = {document: doc}; + const bootstrap = isStandalone + ? renderApplication(MyServerFailingConstructorAppStandalone, options) + : renderModule(MyServerFailingConstructorAppModule, options); + await expectAsync(bootstrap).toBeRejectedWith( + 'Error in constructor of the root component', + ); + expect(getPlatform()).withContext('PlatformRef should be destroyed').toBeNull(); + expect(wasServiceNgOnDestroyCalled) + .withContext('DestroyableService.ngOnDestroy() should be called') + .toBeTrue(); + }, + ); }); });