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.

Sign up for GitHub

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

Jump to bottom

fix(core): unsubscribe every possible events #611

Merged
merged 13 commits into from
Oct 29, 2020
Merged

fix(core): unsubscribe every possible events #611

merged 13 commits into from
Oct 29, 2020

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented Oct 22, 2020 •
edited

@ghiscoding
Copy link
Owner Author

@tannenbaums
I made some more changes, I found all the ngx-translate subscriptions weren't unsubscribe and I reverted all the Subject .complete because we should technically only unsubscribe a subscriptions.

There is only 1 more subscription that I'm not sure how to unsubscribe since it's in the App Module (appInitializerFactory) on this line. I'm not sure if it's even possible but that is a possible left open one.

So give that another try and see if latest commits help. Thanks

@codecov
Copy link

codecov bot commented Oct 26, 2020 •
edited

Codecov Report

Merging #611 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #611    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          149       149            
  Lines         9974      9980     +6     
  Branches      3528      3381   -147     
==========================================
+ Hits          9974      9980     +6
Impacted Files Coverage Δ
...lickgrid/components/angular-slickgrid.component.ts 100.00% <100.00%> (ø)
...rid/extensions/cellExternalCopyManagerExtension.ts 100.00% <100.00%> (ø)
...es/angular-slickgrid/filters/autoCompleteFilter.ts 100.00% <100.00%> (ø)
...r-slickgrid/services/groupingAndColspan.service.ts 100.00% <100.00%> (ø)
...s/angular-slickgrid/services/pagination.service.ts 100.00% <100.00%> (ø)
...ules/angular-slickgrid/services/resizer.service.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a613d4...a18fd45. Read the comment docs.

@tannenbaums
Copy link

tannenbaums commented Oct 26, 2020 via email •
edited

@tannenbaums
Copy link

tannenbaums commented Oct 26, 2020 via email •
edited

@tannenbaums
Copy link

tannenbaums commented Oct 26, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

Sure I can change them to loop through subscriptions array, I'll do that tomorrow but I doubt that would make much difference in this case though.

@ghiscoding
Copy link
Owner Author

ghiscoding commented Oct 27, 2020 •
edited

@tannenbaums
I made the changes you asked, but since you said the 1st Example (basic grids) is no longer showing the issue, I wonder if it could be the Resizer Service causing the issue (it has a body resize binding event which is supposed to unbind on disposing of the Service). So maybe if you could try toggling between these Examples (1, 6, 10, 15, 20) that don't have the auto-resize enabled and see if those grids are having the leak or not. Thanks

@tannenbaums
Copy link

tannenbaums commented Oct 27, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

Well I can't go further than that, so if you find something else than please share it, else I will merge the PR as it is in the coming days.

@tannenbaums
Copy link

tannenbaums commented Oct 27, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

ghiscoding commented Oct 27, 2020 •
edited

Sure I would of course rather fix any memory leaks as much as possible. When you said only Example 10 got fixed, are the other grids without auto-resize still leaking (Examples 1, 6, 10, 15, 20)? I think you have only confirmed Examples 1 and 10 so far, not sure about the others. I also only rewrote the Example 4 to use an ngOnDestroy and call Angular-Slickgrid destroy, I don't think that would be ideal anyway (technically we shouldn't have to call destroy ourselves I would think). Isn't the memory snapshot showing you what events are still attached? That would help to find what the issue is.

@tannenbaums
Copy link

tannenbaums commented Oct 27, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

ghiscoding commented Oct 27, 2020 •
edited

@tannenbaums
I found 2 other possible leaks (most probably have a big effect)

  1. an event that was bound to document (onKeyDown) that was inside the ExternalCopyManager plugin which was enabled in about 10 examples via the enableExcelCopyBuffer flag.
  2. inside the multiple-select.js 3rd party lib that I use for Single/Multiple Select Filters/Editors, I customize that external lib by adding an onClick on the body so that clicking anywhere on the body would close the dropdown but it I forgot to unbound it when I did that code change some time ago (now fixed).

The fact that that both were bound to document/body were most probably a big memory leak, especially number 2 I would assume since it's used in most grids.

You can give another try, that should help a bit more

@tannenbaums
Copy link

tannenbaums commented Oct 28, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

Well I am done on my side, there's nothing else for me to search for, I have fixed everything I could find.

What you could do to help though is go on these Examples and disable all the grid option flags to disable most extensions/plugins and see if it stops leaking then re-enable some of these flags to find what extension/plugin might be leaking. You can start by disabling enableExcelCopyBuffer and enableAutoResize.

Ghislain Beaulac added 2 commits October 28, 2020 09:42
refactor: write code in ES5 syntax to be better supported
1e27e29
chore: refactor code of jQuery unbind which doesn't need method ref
119e738 
- the 2nd argument is actually a selector while the method ref should be on 3rd argument but again it's not mandatory with jQuery and will remove everything when not provided which is what we want
@tannenbaums
Copy link

tannenbaums commented Oct 28, 2020 via email •
edited

@tannenbaums
Copy link

tannenbaums commented Oct 28, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

ghiscoding commented Oct 28, 2020 •
edited

hmm that is that, under the hood the column onCellClick is a connected to the SlickGrid onClick event, I'm using the SlickGrid Event Handler to subscribe (here) and unsubscribeAll (here) and if I look at the Angular-Slickgrid component, I am disposing (destroying) of that GridEvent Service properly on this line) and everything looks fine from my point of view, that is strange.

So as I wrote in first paragraph, technically speaking the onCellClick is a connected to the SlickGrid onClick event.
Could you try to take 1 of them that leaks and replace the column onCellClick with the regular SlickGrid event and move the code into a separate method onCellClicked like so:

<angular-slickgrid gridId="grid22" (onAngularGridCreated)="angularGridReady($event)"
                       (sgOnClick)="onCellClicked($event.detail.eventData, $event.detail.args)"
                       [columnDefinitions]="columnDefinitions" [gridOptions]="gridOptions" [dataset]="dataset">
    </angular-slickgrid>

I'd like to know if it's really that GridEventService causing the issue

If you remove the line in italic, it’ll stop leaking.
Right now my only way to resolve this is declare the method outside the component, which works, but I hope there’s a better solution…

BTW, I don't see any italic line in your GitHub reply, maybe because you replied by email, so I'm not fully sure what you mean in this quote, would you mind pasting the code change you did. Also the column onCellClick and onCellChange are only found in a total of 4 Examples but you provided a longer list of Examples still leaking yesterday, so it might not be only the only problem left.

@tannenbaums
Copy link

tannenbaums commented Oct 28, 2020 via email •
edited

@tannenbaums
Copy link

tannenbaums commented Oct 28, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

ghiscoding commented Oct 28, 2020 •
edited

I can't see your attachment, probably because you replied by email, you should use GitHub website to add print screen, you can run npm run build if you don't use Yarn.

I'd like to confirm if you tried with the regular SlickGrid onClick method via the html, does that leak too? If it doesn't then there's something wrong with the GridEventService and I'd like to confirm that.

<angular-slickgrid gridId="grid22" (onAngularGridCreated)="angularGridReady($event)"
                       (sgOnClick)="onCellClicked($event.detail.eventData, $event.detail.args)"
                       [columnDefinitions]="columnDefinitions" [gridOptions]="gridOptions" [dataset]="dataset">
</angular-slickgrid>

@tannenbaums
Copy link

tannenbaums commented Oct 29, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

ahh yeah I remember that problem, I had it too, I think it's because I need a newer version of TypeScript in the project because, for some unknown reasons it updated to latest version of @types/babel__traverse which is an indirect dependency so I don't have any control to downgrade it. I had to take an older version of @type/babel and overwrite the one I have in Angular-Slickgrid...

long story short, it's probably easier if I release a version tomorrow

@tannenbaums
Copy link

tannenbaums commented Oct 29, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

I found out that if we overwrite the index.d.ts file of Angular-Slickgrid\node_modules\@types\babel__traverse with the attached index.d.ts file (an older version) it finally compiles. From what I found by reading this SO question, newer TypeScript changed the output of index.d.ts and my lib uses an older TypeScript version because the lib project is still under Angular 7.

babel__traverse.zip

@ghiscoding ghiscoding changed the title fix(core): every Subject should be destroyed, fixes #610 fix(core): unsubscribe every possible events Oct 29, 2020
@ghiscoding ghiscoding merged commit 2a92e78 into master Oct 29, 2020
@ghiscoding ghiscoding deleted the fix/memory-leak branch October 29, 2020 13:33
@ghiscoding
Copy link
Owner Author

I pushed a new patch version "angular-slickgrid": "^2.22.5", now I just need to spend a few minutes to update all the Angular-Slickgrid-Demos with the changes done in the Examples. You should be good to go, I'm not sure what to do with the GridEventService that has the column onCellClick, you said it doesn't leak when using the event inside the View (html), I could always deprecate it in the next major version but I find it useful to declare it right in the column definitions... anyway, if it fixes all your leak then that's a big push in the right direction

@tannenbaums
Copy link

tannenbaums commented Oct 29, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

I pushed the changes under latest version 2.22.5, if you use the carrent ^ then it will update all minor (the 2nd number 22), if you use ~ it will update the last number 5

@tannenbaums
Copy link

tannenbaums commented Oct 29, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

ghiscoding commented Oct 29, 2020 •
edited

I don't think that the version of Angular I use has anything to do with that, to create the lib package I use ng-packagr, that produces FESM2015 and UMD formats. If there's anything, it could be ng-packgr not really Angular itself. However I made a PR last night to bump the project to Angular 8 which also has a bit newer version of ng-packagr, it also uses a newer version of TypeScript (it was mainly to fix the ton of errors message with @types/babe__traverse that you saw yesterday)... so you can try that PR #617 and run the yarn run build and copy the dist folder content and overwrite your node_modules/angular-slickgrid folder and see if that helps.

I'm not sure that I want to go over Angular 8 in the lib project yet, I know they changed to Ivy in Angular 9 so I wanted to stick with Angular 8 for some time. The PR #617 for Angular 8 is not merged yet, I'll do a future release as a minor (so that would 2.23.0) but I'm not in a rush, unless you tell me it helps (which I doubt). If that doesn't fix it and you have some time, you can try upgrading the lib project to Angular 10 and if that does fix the leaking then I'll go for it.

@tannenbaums
Copy link

tannenbaums commented Oct 29, 2020 via email •
edited

@ghiscoding
Copy link
Owner Author

Every time you post a print screen of attachment via an email reply I can't see it in GitHub, so I don't know what you posted. If you have a way to fix whatever you posted, please make a PR, there isn't much more I can do on my side. Thanks

@tannenbaums
Copy link

So sorry I keep forgetting, ok now I'm replying on the post. I didn't realize it moved to a different thread than the one I started that's what got me confused...

image

@ghiscoding
Copy link
Owner Author

Ok I now see the image but it's too small, I can't see the text lol... but still I don't know what else to do. What are the grid options that you're using with that grid? I assume you use some of the features.

@tannenbaums
Copy link

Here's a better image, it's really important, it shows where the leak is:

image

@ghiscoding
Copy link
Owner Author

That is a great reference but I still don't have a clue on how to fix that, I don't know Angular in depth enough (I understand the ViewRef concept but I don't understand what is good and what is wrong) so I don't know what to know (in fact I haven't used Angular in couple months, we are using something else for current project). If you have a way to fix it, please please make a PR.

@tannenbaums
Copy link

Well in AngularUtilService you are assigning a viewRef I thought it has todo with that...no?

@tannenbaums
Copy link

you have a line like this:

this.appRef.attachView(componentRef.hostView);

this looks like it has a lot to do with this issue...but i might be wrong...
maybe there's a way to release it when component is destroyed...

@ghiscoding
Copy link
Owner Author

So you're using the Row Detail then, if you see in the AngularUtilService, there's a link which point to what I found to dynamically load an Angular Component into the Row Detail. The link is Angular Pro Tip: How to dynamically create components in . I did not analyze the code much, I just reuse what was in the article to get the Row Detail working (AngularUtilService is mainly used for Row Detail only).

@tannenbaums
Copy link

tannenbaums commented Oct 29, 2020 •
edited

All I have is an html template which has this:
<angular-slickgrid> </angular-slickgrid>
that's it!! no other code (the ts file is just an empty constructor)!! and that's causing the image that I sent.

@ghiscoding
Copy link
Owner Author

Well I don't know why because as I said that ViewRef that you found is really only used by the Row Detail (I search in the lib), so I don't know why it creates that structure.

@tannenbaums
Copy link

I have very good news!!! I didn't test it properly this morning, must have been some sort of a refresh/caching issue.
I retested and NO MORE LEAK!!!! now that's exciting news!!!!!

Thank you so much for all your on-going help and support!!

@ghiscoding
Copy link
Owner Author

that is indeed excellent news, thanks for all the communication and feedback 🚀 😄

@ghiscoding ghiscoding mentioned this pull request Oct 30, 2020
Closed
@tannenbaums
Copy link

tannenbaums commented Nov 4, 2020 •
edited

Hi,

I just opened up another issue. After you fixed the memory leak last week, although I was able to see that my components now are only loaded once, but to my dismay I was still having problems. So I continued digging and now I believe I finally found the cause: there's still another type of memory leak called DOM Leak. If you can take a look I would greatly appreciate. This time I was able to confirm that once it's fixed on your end, my app should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ghiscoding @tannenbaums