fix(memory leak): memory leaking in UniqueSelectionDispatcher #5164
Conversation
googlebot
added
the
cla: yes
|
Please merge ASAP: it is causing huge mem-leaks in my application so it makes the Chrome tab to crash in few minutes... In combination with #5144 I'm almost unable to use the library. We are managing a big data set that is leaking because of these... |
| * Listen for future changes to item selection. | ||
| * @return Function used to unregister listener | ||
| **/ | ||
| listen(listener: UniqueSelectionDispatcherListener): Function { |
| @@ -305,6 +306,9 @@ export class MdButtonToggle implements OnInit { | |||
| /** Whether or not the button toggle is a single selection. */ | |||
| private _isSingleSelector: boolean = null; | |||
|
|
|||
| /** Unregister function for _buttonToggleDispatcherListener **/ | |||
| private _buttonToggleDispatcherListener: Function; | |||
There was a problem hiding this comment.
Call this _removeUniqueSelectionListener, type is () => void and initialize it to () => {}:
private _removeUniqueSelectionListener: () => void = () => {};Then you don't have to check if it is defined in the ngOnDestroy
(same change for other components as well)
@devversion this is potentially a good case for a mixin in a follow-up PR
| this.checked = false; | ||
| } | ||
| }); | ||
| this._buttonToggleDispatcherListener = _buttonToggleDispatcher.listen( |
There was a problem hiding this comment.
Prefer line-breaking at the higher syntactic level, e.g.
this._removeUniqueSelectionListener =
_buttonToggleDispatcher.listen((id: string, name: string) => {
if (id != this.id && name == this.name) {
this.checked = false;
}
});| return () => { | ||
| this._listeners = this._listeners.filter((registered: UniqueSelectionDispatcherListener) => { | ||
| return listener !== registered; | ||
| }); |
There was a problem hiding this comment.
This also needs a unit test. It should set up a listener, then deregister and assert that listener isn't called again on notify
There was a problem hiding this comment.
Done. See unique-selection-dispatcher.spec.ts.
|
@jelbourn All comments are implemented. |
Eddman
changed the title
jelbourn
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Listeners are only registered inside of
UniqueSelectionDispatcher. But there were never removed causing in my case a small memory leak when usingMdButtonToggle.In this fix
UniqueSelectionDispatcher.listenreturns a function that removes the listener once called.