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

Angular-Slickgrid is causing a memory leak in my application #610

Closed
tannenbaums opened this issue Oct 21, 2020 · 11 comments
Closed

Angular-Slickgrid is causing a memory leak in my application #610

tannenbaums opened this issue Oct 21, 2020 · 11 comments

Comments

@tannenbaums
Copy link

tannenbaums commented Oct 21, 2020

I'm submitting a Bug report

Your Environment

Software Version(s)
Angular 10.1.6
Angular-Slickgrid 2.22.4
TypeScript 4.0.3

Describe the Bug

It's been a couple of days that I'm trying to figure out why certain aspects of my angular application are slow. After much hassle I finally figure out that my destroyed components are not getting unloaded from memory. I use the memory tab in chrome tools, I take a snapshot of the heap and all my components are times the amount of time I switched between views, which is a big problem because this slows down the application a lot.
So I decided I'm going to take out most of the code and just have the skeleton of the components and have the ability to route between them and I kept removing code until the problem was gone. It turns out that as long as I don't add angular slick grid to my html component I'm fine. As soon as I add it, this problem happens.
It usually means that you might have even listeners that are still open, or something like that, that's preventing the GC to unload them from memory.
As a matter of fact I downloaded the Angular-Slickgrid project and ran the examples locally, and than ran the same memory test. And sure enough same problem the components keep piling up as I switch between them.

image

The strange thing is that if I compile Angular-Slickgrid in prod mode, I don't see any components in the heap (meaning to say I don't see any objects that end with the word component) nor do I see any objects that resemble the components in examples, why is that? The only thing I can find is SlickGrid and every click adds another instance in prod build as well.

If you can shed some light into this would be very much appreciated, I'm at loss what to do next.

Steps to Reproduce

as mentioned above just run the examples, keep toggling between components a couple of times. Then hit F12 to bring up Chrome Dev Tools, go to the memory tab, in the Heap snapshot option there's a Take snapshot button, hit that. Once the snapshot is loaded, you can filter by component, and you'll see that

Expected Behavior

Destroyed components should be garbage collected and should not persist in memory.

Current Behavior

Components are not being removed from the heap.

Possible Solution

Code Sample

The Angular-Slickgrid sample shows this problem

@ghiscoding
Copy link
Owner

ghiscoding commented Oct 21, 2020

You can try destroying the component by using this.angularGrid.destroy() inside an ngDestroy which will dispose of all the Angular-Slickgrid Services and everything else. See if that helps, if not then I don't know what else to try.

angularGridReady(angularGrid: AngularGridInstance) {
    this.angularGrid = angularGrid;
}

ngOnDestroy() {
    this.angularGrid.destroy();
}

I have never used this memory tab in Chrome and never knew how to use it (but I never looked for it either). I added the ngDestroy in the GridClientSideComponent (Example 4) and then I read your instructions a bit fast (bcoz it's end of a long day) but if I toggle back and forth between first 4 grids a few times then take a snapshot, I don't see the GridClientSideComponent but I see the other ones, is that the heap test? If so does that fix it? Give that a shot

image

@tannenbaums
Copy link
Author

Thank you so much for your quick response. Unfortunately, that doesn't fix the problem. It's interesting though, as you pointed out and I ran some tests on my end as well, the GridClientSideComponent always stays one instance, so I'm trying to figure out what can be the line of code that sets this component apart from the others, tried a couple of things but no dice yet.
Just wanted to say this: If anywhere in Angular-Slickgrid code there's a Observable that has an open subscription (unsubscribe wasn't called on it), or any thing of the like, meaning any open listeners, it'll cause the entire component tree to stay in memory. Do you know if it's possible that this type of thing is happening here? Just fyi: I have a base class that all my components derive from which provides an api to open up a subscription, and then it auto closes all of them at destroy time...
Any help would be invaluable as this is causing serious issues in production and my users are eagerly waiting for a fix. I'm already on this for almost a week without an end in site :(

Thank you so much!

@ghiscoding
Copy link
Owner

Hmm looking at some Services, there a few Subject which I didn't unsubscribe from, for example this line in Filter Service. That would help if you can clone the lib and try to unsubscribe all the Subject, it seems that none of them are unsubscribed

ghiscoding-SE pushed a commit that referenced this issue Oct 22, 2020
- causes memory heap size to keep growing
@ghiscoding
Copy link
Owner

ghiscoding commented Oct 22, 2020

I made some changes to make sure to unsubscribe by using $subject.complete() in all of my Services, if you could give that a try (from the PR which is not merged) that would help since you seem to know how to test it.

I see that the heap size stays about the same now, not sure if that's a valid test, here's what it looks like after toggling about 10x times back and forth between some of the Examples

image

Actually this is probably not a good test since the GitHub Demo seems to be behave the same way.

@tannenbaums
Copy link
Author

tannenbaums commented Oct 22, 2020

Thank you, much appreciated!! It's still a problem though, and looking at the code I can see that there are still a bunch of subscribe calls that the return value is not assigned to any variable, which means it cannot be unsubscribed from such as this in grid-frozen:

this.gridObj.onMouseEnter.subscribe(event => {
  const cell = this.gridObj.getCellFromEvent(event);
  this.gridObj.setSelectedRows([cell.row]); // highlight current row
  event.preventDefault();
});

. Also are you sure that complete does the job? From the little research that I did it seems like complete will make sure to complete the event handler, but it will continue to listen to future events.

@ghiscoding
Copy link
Owner

ghiscoding commented Oct 22, 2020

No I'm not sure for complete, I never used it this way, I simply found it on this article and this PR seems to be causing some issues in other areas like Pagination. Need to test this more.

I would prefer if you have time to look at the code and find the ones that are unsubscribe and possibly do a PR. I actually haven't in couple months, working on something else for now.

It's possible that a few Examples are missing unsubscribed, that Frozen one you said is missing it and should be updated with

ngOnDestroy() {
    this.gridObj.onMouseEnter.unsubscribe();
    this.gridObj.onMouseLeave.unsubscribe();
}

@tannenbaums
Copy link
Author

Yes, will do, thanks again!

@ghiscoding
Copy link
Owner

I could be wrong but aren't suppose to unsubscribe only something we subscribed to? Those subscriptions should be handled already, at least for the most part. I might have misunderstood how RxJS works though, so any PR is welcome.

@tannenbaums
Copy link
Author

I hear, I'm not sure. The fact is that those are the only ones left, that are not unsubscribed from, and it's still leaking, so I'm hoping that will fix it...we'll see

ghiscoding added a commit that referenced this issue Oct 29, 2020
* fix(core): unsubscribe every possible events #610
- causes memory heap size to keep growing

* refactor: use subscriptions array instead of single subscription

* fix: remove keydown binding when disposing of externalCopy plugin

* fix: possible leak, body onClick event not unbound in multiple-select
@ghiscoding
Copy link
Owner

ghiscoding commented Oct 30, 2020

From your last comment I can assume that we can now close the issue 🚀 👾

You can also upvote if you haven't already ⭐
Cheers 🎉

@tannenbaums
Copy link
Author

tannenbaums commented Nov 2, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants