Skip to content

Conversation

Eddman
Copy link
Contributor

@Eddman Eddman commented Sep 1, 2017

The subscription to this._ngZone.onMicrotaskEmpty is causing memory leaks:
image

@Eddman Eddman requested a review from andrewseguin as a code owner September 1, 2017 08:27
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 1, 2017
@jelbourn
Copy link
Member

@andrewseguin should we revisit this?

@andrewseguin
Copy link
Contributor

Yup - this still is valid I believe, since the subscription is not always unsubscribed

We now have four subscriptions in tooltip. I think we can benefit from an onDestroyed stream that we should takeUntil

@Eddman
Copy link
Contributor Author

Eddman commented Jan 30, 2018

@andrewseguin @jelbourn In my case I have a requirement to display table with tooltips in cells. With this amount of tooltips multiplied by size of data in memory we generate a huge leak. So yes this is still valid :)

Anyway I agree this was my first quick draft. Do you want me to implement it differently? Or at least rebase this branch?

@jelbourn
Copy link
Member

Updating this to use destroyed subject along with takeUntil would be good (vs. keeping track of the subscription).

@andrewseguin
Copy link
Contributor

An example of what Jeremy was pointing out can be found here: https://github.com/angular/material2/blob/40a507043a47861d1d0f66dc5eb7591d762b089b/src/lib/sidenav/drawer.ts#L495

@Eddman Eddman requested a review from jelbourn as a code owner January 31, 2018 08:43
@Eddman
Copy link
Contributor Author

Eddman commented Jan 31, 2018

@jelbourn @andrewseguin I've implemented the fix using takeUntil. Tested in our app. It works without issues or leaks 😉

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Feb 1, 2018
@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Feb 9, 2018
@mmalerba
Copy link
Contributor

mmalerba commented Feb 9, 2018

@Eddman please rebase

@Eddman
Copy link
Contributor Author

Eddman commented Feb 10, 2018

@mmalerba Done.

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Feb 10, 2018
@jelbourn jelbourn added the target: patch This PR is targeted for the next patch release label Feb 13, 2018
@jelbourn jelbourn merged commit 66a01fb into angular:master Feb 13, 2018
@Eddman Eddman deleted the tooltip_mem_fix branch February 13, 2018 18:05
@andrewseguin andrewseguin added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Feb 20, 2018
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants