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

Memory leak when hotTable's parent scope is destroyed. #157

Open
Ruffle opened this issue Apr 26, 2016 · 6 comments
Open

Memory leak when hotTable's parent scope is destroyed. #157

Ruffle opened this issue Apr 26, 2016 · 6 comments

Comments

@Ruffle
Copy link

Ruffle commented Apr 26, 2016

Hi,
I often use the directive inside controllers whoose scope is later destroyed. I'm not an expert but it seems that, in this case, the elements are not properly removed from the DOM which causes a memory leak.
I discovered that, in the vanilla handsontable code, you can call the .destroy() method on a hot instance to remove it from the DOM so I added
element.on("$destroy", function() { scope.hotInstance.destroy(); });
at the end of hotTable's "compile" (line 584), which fixed the memory leak.

I don't know if this could break other parts of your code but I just wanted to let you know there's potential problem here.

@Rex90
Copy link

Rex90 commented Aug 4, 2016

I'm also having serious leakes because i destroy my controllers holding this directive. i tried adding what you said but I get this error when i toggled routes (in ui router)

angular.js:13424 Error: This method cannot be called because this Handsontable instance has been destroyed
    at Core.postMortem (handsontable.full.js:5962)
    at Object.updateHandsontableSettings (ngHandsontable.js:157)
    at removeColumnSetting (ngHandsontable.js:475)
    at ngHandsontable.js:440
    at Scope.$broadcast (angular.js:17348)
    at Scope.$destroy (angular.js:16966)
    at cleanOld (angular-ui-router.js:3978)
    at angular-ui-router.js:3984
    at processQueue (angular.js:15757)
    at angular.js:15773

@terite
Copy link

terite commented Aug 17, 2016

👍 this directive needs to clean up after itself using the element.on("$destroy" method shown in the ticket.

Right now we have to work around that by manually destroying hotInstance using the hotRegisterer

@Ruffle
Copy link
Author

Ruffle commented Aug 17, 2016

@armensg I tested the line of code I previously mentioned and I got a similar error. I don't remember under what circumstances it worked (just got back from holidays ^^) but there are other ways to solve this.

Just as terite pointed out you have to destroy the hotInstance manually using hotRegisterer. I searched my code and It looks like what I ended up doing was creating another directive which you can add to <hot-table> as an attribute:

angular.module('app').directive('hotAutoDestroy',["hotRegisterer",function(hotRegisterer) {
    return {
        restrict: 'A',
        link: function (scope, element, attr){
            element.on("$destroy", function() {
                var hotInstance = hotRegisterer.getInstance(attr.hotId);
                hotInstance.destroy();
            });
        }
    }
}]);

then:

<hot-table hot-auto-destroy>

@Rex90
Copy link

Rex90 commented Oct 3, 2016

@Ruffle0 actually I am not sure its working. I still have performance drop after and many events not getting removed. I modified the directive a bit to remove the post-mortem message.

angular.module('dealer.directives').directive('hotAutoDestroy',["hotRegisterer",function(hotRegisterer) {
    return {
        restrict: 'A',
        link: function (scope, element, attr){
            element.on("$destroy", function() {
                try{
                    console.debug("Going to destroy hot instance");
                    var hotInstance = hotRegisterer.getInstance(attr.hotId);
                    if(!isNaN(hotInstance)){
                       hotInstance.destroy(); 
                    }
                    else{
                        // this is when post mortem message is thrown.
                        console.log("Failed to destroy hot-instance");
                    }
                }
                catch(er){
                    console.log(er);
                }
            });
        }
    };
}]);

@Rex90
Copy link

Rex90 commented Oct 6, 2016

would be useful if there as a function to remove event listeners from the page - since the destrory function is failing to do this.

@wtfdaemon
Copy link

Did this ever get resolved? We're seeing something similar and it's keeping us from using handsontable. Shouldn't destroy() remove the event listeners explicitly?

budnix added a commit that referenced this issue Aug 19, 2020
When the scope is destroyed or the DOM element is removed the
Handsontable instance is destroyed.

Issue: #157
@budnix budnix mentioned this issue Aug 19, 2020
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

5 participants