Skip to content

fix(angular): prevent memory leak when the root view is removed #3594

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.

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

Merged
merged 1 commit into from
May 25, 2021
Merged

fix(angular): prevent memory leak when the root view is removed #3594

merged 1 commit into from
May 25, 2021

Conversation

arturovt
Copy link
Contributor

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

The TraceService creates multiple subscriptions to router.events and never unsubscribes. The root view may be created and destroyed many times in different cases:

  • micro-frontend app (the AppModule is bootstrapped and destroyed through NgModuleRef.destroy() whenever the app should be rendered or destroyed)
  • server-side rendering

In the above cases, this keeps creating the TraceService for each new app, but it also prevents many resources from being GC'd, for instance TraceService, AppModule (since its injector keeps the reference), Router, etc.

@arturovt arturovt requested a review from kamilogorek as a code owner May 25, 2021 10:39
@kamilogorek kamilogorek enabled auto-merge (squash) May 25, 2021 10:45
@kamilogorek
Copy link
Contributor

Makes perfect sense. Thanks!

auto-merge was automatically disabled May 25, 2021 10:45

Pull request was closed

@kamilogorek kamilogorek reopened this May 25, 2021
@kamilogorek kamilogorek enabled auto-merge (squash) May 25, 2021 10:45
@kamilogorek
Copy link
Contributor

Whoops, wrong button.

@kamilogorek
Copy link
Contributor

2 small linter issues, otherwise approved.

auto-merge was automatically disabled May 25, 2021 11:09

Head branch was pushed to by a user without write access

@kamilogorek kamilogorek enabled auto-merge (squash) May 25, 2021 11:30
@kamilogorek kamilogorek merged commit d1d6c41 into getsentry:master May 25, 2021
@arturovt arturovt deleted the fix/angular-leak branch May 25, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants