fix(platform-server): destroy `PlatformRef` when error happens during the `bootstrap()` phase, e.g. in `APP_INIIALIZER` - to prevent memory leaks in SSR by Platonn · Pull Request #58112 · angular/angular

Platonn · 2024-10-07T21:12:42Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
[N/A] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.dev application / infrastructure changes
Other... Please describe:

What is the current behavior?

PlatformRef is not destroyed if an error happens during the bootstrap() phase (e.g. in APP_INITIALIZER). Then the main app's injector is not destroyed and therefore ngOnDestroy hooks of singleton services were not called on the end (failure) of SSR.

Note: 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).

Issue Number: #58111

What is the new behavior?

PlatformRef is guaranteed to be destroyed in renderApplication() and renderModule() functions of @angular/platform-server both in the case of successful render and in the case of a failure (resulting in a rejected promise)
PLATFORM_DESTROY_LISTENERS that destroys the app's main injector is now set not only in case of renderApplication(), but also in case of renderModule()

Does this PR introduce a breaking change?

Yes
No

Other information

JeanMeche · 2024-10-07T21:51:13Z

Hi Krzysztof, thanks for looking into this. Could you please rebase your PR on the main branch (so we can run the CI)

Platonn · 2024-10-07T21:54:53Z

Thank you @JeanMeche for your reply.
I've rebased with main just now and resolved the code conflicts.

packages/platform-server/test/integration_spec.ts

JeanMeche · 2024-10-07T22:19:39Z

The changes are breaking the elements pacakge, can you have a look ?

Platonn · 2024-10-07T23:01:40Z

Thank you @JeanMeche for letting me know.
Now pushed a fix.

Explanation:

Before this PR
- the isApplicationBootstrapConfig() was duck-typing based on the presence of the property platformInjector.
In this PR initially
- I've changed the logic to check the property rootComponent instead, because I wanted to pass the platformInjector in all cases - also in the NgModule flow (in order to setup the PLATFORM_DESTROY_LISTENER later in the bootstrap() function).
- It turned out to break the behavior of the public API function createApplication() which was tested inside the elements package. The test failed, because no rootComponent was passed in that test case setup as a param of createApplication. Then isApplicationBootstrapConfig() returned false which led into incorrectly executing the NgModule-flow of bootstrap(). Despite someone could perceive omitting the rootComponent as an edge case usage of createApplication(), I broke that default behavior. So needed to fix it
The fix
- So now I've fixed the problem by no-longer duck-typing inside the isApplicationBootstrapConfig() function. Instead, I started passing explicitly a newly-introduced config property BootstrapKind as a required param of the bootstrap() function. btw. It's not a breaking change, because the signature of the bootstrap() function is not a part of the public API, as far as I can understand.

packages/core/src/platform/bootstrap.ts

alan-agius4

LGTM, just a couple of NITs

packages/core/src/platform/bootstrap.ts

packages/platform-server/src/utils.ts

… 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 angular#58111

AndrewKushnir · 2024-10-08T21:21:53Z

@Platonn thanks for creating this PR 👍 It looks like there is a failing test (see the test CI job output). Could you please take a look at it when you get a chance?

JeanMeche · 2024-10-08T21:33:11Z

@AndrewKushnir It looks like the failing test is flaky. CI is fine after a re-run.
I couldn't reproduce the test-breakage locally.

AndrewKushnir · 2024-10-09T04:41:00Z

Presubmit.

angular-automatic-lock-bot · 2024-11-09T00:10:51Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pullapprove bot requested a review from pkozlowski-opensource October 7, 2024 21:12

Platonn mentioned this pull request Oct 7, 2024

PlatformRef is not destroyed in SSR if error happens during the bootstrap() phase (e.g. when an APP_INITIALIZER rejects) #58111

Closed

Platonn force-pushed the fix/GH-58111 branch from 1b84662 to 83d6647 Compare October 7, 2024 21:52

JeanMeche reviewed Oct 7, 2024

View reviewed changes

packages/platform-server/test/integration_spec.ts Outdated Show resolved Hide resolved

Platonn force-pushed the fix/GH-58111 branch from 83d6647 to a15a5d0 Compare October 7, 2024 22:06

Platonn force-pushed the fix/GH-58111 branch 2 times, most recently from 22b7bca to a13f1fb Compare October 7, 2024 22:45

JeanMeche reviewed Oct 7, 2024

View reviewed changes

packages/core/src/platform/bootstrap.ts Outdated Show resolved Hide resolved

JeanMeche requested a review from AndrewKushnir October 7, 2024 23:20

Platonn force-pushed the fix/GH-58111 branch from a13f1fb to ac7fb42 Compare October 8, 2024 05:28

alan-agius4 approved these changes Oct 8, 2024

View reviewed changes

packages/core/src/platform/bootstrap.ts Outdated Show resolved Hide resolved

packages/platform-server/src/utils.ts Outdated Show resolved Hide resolved

packages/platform-server/src/utils.ts Outdated Show resolved Hide resolved

Platonn force-pushed the fix/GH-58111 branch from ac7fb42 to 0d957c2 Compare October 8, 2024 14:27

AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: server Issues related to server-side rendering target: patch This PR is targeted for the next patch release labels Oct 8, 2024

ngbot bot added this to the Backlog milestone Oct 8, 2024

AndrewKushnir approved these changes Oct 9, 2024

View reviewed changes

AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 9, 2024

AndrewKushnir removed the request for review from pkozlowski-opensource October 9, 2024 04:41

AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Oct 9, 2024

davidjbradshaw mentioned this pull request Nov 2, 2024

[Snyk] Upgrade @angular/core from 18.2.8 to 18.2.10 davidjbradshaw/iframe-resizer#1356

Merged

kinkaz mentioned this pull request Nov 6, 2024

Issue with SSR: intermittent errors under load #58526

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(platform-server): destroy `PlatformRef` when error happens during the `bootstrap()` phase, e.g. in `APP_INIIALIZER` - to prevent memory leaks in SSR#58112

fix(platform-server): destroy `PlatformRef` when error happens during the `bootstrap()` phase, e.g. in `APP_INIIALIZER` - to prevent memory leaks in SSR#58112
Platonn wants to merge 1 commit intoangular:mainfrom
Platonn:fix/GH-58111

Platonn commented Oct 7, 2024 •

edited

Loading

Uh oh!

JeanMeche commented Oct 7, 2024

Uh oh!

Platonn commented Oct 7, 2024

Uh oh!

Uh oh!

JeanMeche commented Oct 7, 2024

Uh oh!

Platonn commented Oct 7, 2024 •

edited

Loading

Uh oh!

Uh oh!

alan-agius4 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented Oct 8, 2024

Uh oh!

JeanMeche commented Oct 8, 2024 •

edited

Loading

Uh oh!

AndrewKushnir commented Oct 9, 2024

Uh oh!

angular-automatic-lock-bot bot commented Nov 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Platonn commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

JeanMeche commented Oct 7, 2024

Uh oh!

Platonn commented Oct 7, 2024

Uh oh!

Uh oh!

JeanMeche commented Oct 7, 2024

Uh oh!

Platonn commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented Oct 8, 2024

Uh oh!

JeanMeche commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewKushnir commented Oct 9, 2024

Uh oh!

angular-automatic-lock-bot bot commented Nov 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Platonn commented Oct 7, 2024 •

edited

Loading

Platonn commented Oct 7, 2024 •

edited

Loading

JeanMeche commented Oct 8, 2024 •

edited

Loading