DRAFT: feat(memory-leak): add elements cleanup for did-intersect modifier #1078
Conversation
| let originalRemoveElement = this.observerManagerService.removeElement; | ||
| const observerManagerService = this.observerManagerService; | ||
|
|
||
| observerManagerService.removeElement = function removeElementExt( |
There was a problem hiding this comment.
I think this would be better accomplished with sinon.spy - it will still call the underlying function and then you can check the call count on the spy in your assertion.
| @@ -8,6 +8,11 @@ export const DEFAULT_OBSERVER_OPTIONS = {}; | |||
|
|
|||
| function cleanup(instance) { | |||
| instance.unobserve.call(instance); | |||
| instance.observerManager.removeElement(instance.element); | |||
| instance.observerManager.removeElement(window); | |||
There was a problem hiding this comment.
Why window cleanup here? This modifier doesn't explicitly subscribe to window at all.
There was a problem hiding this comment.
Yes, I know it's weird. I'll be taking this to the interesction-observer-admin next.
I'd hope that this addon won't need these changes actually, since the issue lies on the dependency it uses.
But since I've had this experimental patch already, I decided I'd open a PR to showcase the issue. And frankly, I didn't think I'd get your attention so fast 😅
There was a problem hiding this comment.
Heh! Well, I may not have had a lot of time for active development, but I very much appreciate contributions and will do my best to be responsive (though I'm on vacation for a week starting Saturday, FYI). :)
I definitely think some fixes to the underlying dependency make sense in the context of this issue. If this is a workaround until you can get those fixes in, can you add a brief comment explaining why the workaround is needed, and then we can remove it once you land fixes upstream?
| @@ -23,4 +23,9 @@ export default class ObserverManagerService extends Service { | |||
| willDestroy() { | |||
| this._admin.destroy(); | |||
| } | |||
|
|
|||
| removeElement(element) { | |||
| this._admin.elementRegistry.removeElement(element); | |||
There was a problem hiding this comment.
elementRegistry is private. Are you sure we can access it?
https://github.com/snewcomer/intersection-observer-admin/blob/master/src/index.ts#L22
|
|
||
| removeElement(element) { | ||
| this._admin.elementRegistry.removeElement(element); | ||
| this._admin.registry.removeElement(element); |
There was a problem hiding this comment.
Where is registry defined on the intersection-observer-admin class?
There was a problem hiding this comment.
Do we perhaps need to make updates to the underlying class to expose a public method for this kind of cleanup?
There was a problem hiding this comment.
We'll see, I'd imagine that that's what unobserve is for already, so hopefully it can hide all that
There was a problem hiding this comment.
Yea, it seems like maybe there's some extra steps missing from the unobserve implementation in intersection-observer-admin to release these references.
|
|
||
| assert.strictEqual( | ||
| removedElementsCount, | ||
| 49 * 2, // once for the element, once for a window object |
There was a problem hiding this comment.
We should also assert against the underlying IntersectionObserverAdmin to ensure its registry gets cleaned up and prove that your implementation of removeElement works.
There was a problem hiding this comment.
Initially I wanted to inspect the count of elements, but the registry is a WeakMap which doesn't have a length/count method.
There was a problem hiding this comment.
I wonder if you could do something clever like grab a reference to one of the elements (which are the keys to the registry, I think?) and then assert that a get() call on the WeakMap returns undefined after it's been cleaned up? We wouldn't necessarily need to assert all of them, you could set up a separate test that just renders once and make sure that one gets cleaned up. 🤔
There was a problem hiding this comment.
I did think about that but then the issue is that you'd probably want to make sure that you no longer hold this reference yourself, so it'd just get funky 👯
There was a problem hiding this comment.
What I'm thinking is that you grab the reference only in the test. The sinon spy for removeElement should be able to remember what its call arguments were, which you could use to get access to the element, and then you can assert against the registry contents from there.
|
Thanks for identifying this issue, I'd love to get it fixed. I left a few comments, and it looks like there are a few failing tests as well. Feel free to ping me once you have updates and I'll be happy to take another pass at it! |
|
Does this fix in the underlying dependency help you at all in the context of this PR, if it lands? |
|
That could be it, yeah |
|
Alright, I'll see if I can poke Scott about getting that PR landed. If you need additional fixes, feel free to open a PR in that repo as well and I'll tell him to keep an eye out for it. |
BobrImperator
changed the title
|
FYI the current master of For |
|
Closing since |
|
That's great! If you want to open a PR forcing that as the minimum version to bring in the bug fix, I'd be happy to have it. I know we're due for a new release, I've just been pretty busy, but at least you'll be unblocked on your end by just bumping the version in your own app. |
|
@BobrImperator v7.2.0 contains the bumped version of |
Our client has been experiencing severe memory leaks. Which
did-intersectwas partially responsible for.Currently I'm in the process of porting our monkey patches and documenting the issue.
We've also found
ember-in-viewportto be causing these, which in turn is usingintersection-observer-adminjust like this addon.Applying an identical fixes to both addons has fixed our problems.
With fix:
Without fix:
