Skip to content
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

fix(tooltip): memory leak in _setTooltipMessage #6782

Merged
merged 4 commits into from
Feb 13, 2018
Merged

fix(tooltip): memory leak in _setTooltipMessage #6782

merged 4 commits into from
Feb 13, 2018

Conversation

Eddman
Copy link
Contributor

@Eddman Eddman commented Sep 1, 2017

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

@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

@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