angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.8k stars 27.48k forks source link

$destroy on rootScope is ignored #1537

Closed suedama1756 closed 10 years ago

suedama1756 commented 12 years ago

Hi,

$destroy on root scope is ignored. In addition to this, scopes do not appear to go through a $destroy cycle when their corresponding element is destroyed, e.g. they do not subscribe to element.$destroy and forward this on to scope.$destroy. This is a problem as custom directives tend to subscribe to scope.$destroy to perform clean-up rather than element (although you could argue they should). This makes it quite difficult to tear down an angular based "widget" or application. With the ability to manually bootstrap an angular application, it would be great to also have the ability to tear down.

In my case I've got some clean (ish) ways of working around the issue, but would appreciate any suggestions...

petebacondarwin commented 12 years ago

Can you provide a test that shows this (not) happening?

mernen commented 12 years ago

Indeed, the $destroy() method won't work on $rootScope (see here). I've worked around that by invoking $rootScope.$broadcast("$destroy") rather than .$destroy() when eliminating an entire Angular instance on our app. This way, all destructors are invoked the same.

As for the element $destroy event, I have to admit I wasn't even aware of it just a few days ago… I hadn't seen it anywhere in the docs, plus I'm using jQuery so according to #1512 it wouldn't work for me anyway.

suedama1756 commented 12 years ago

I need to do some more digging, however I'm not convinced that broadcasting a destroy is the correct approach as it does not actually call destroy on the scope, e.g. it doesn't unlink it from siblings, parents, $digest cycles, etc. It simply broadcasts the message...., alternatively...

var rootScope = injector.get('$rootScope');

function findBaseScopes() {
    var result = [];
    $('.ng-scope').each(function (i, x) {
        var scope = angular.element(x).scope();
        if (scope.$parent === rootScope) {
            result.push(scope);
        }
    });
    return result;
}

findBaseScopes().forEach(function (scope) {
    scope.$destroy();
});
mernen commented 12 years ago

If you look at the $destroy() implementation, it only does two things:

  1. Broadcast $destroy on that scope
  2. Remove itself from its parent's and siblings' linked lists (no change to its children, they are just left for garbage collection)

Since the $rootScope has no parent or siblings, there's nothing to be done on the second step, so broadcasting the event should be enough. Granted, though, this is not the best future-proof approach.

suedama1756 commented 12 years ago

OK, after some more investigation calling $destroy to tear down angular is currently flawed, ngInclude, etc. don't subscribe to $destroy. The correct way (I believe) to tear down the application is to structure it in such a way that you can effectively clear the model data bound at the root, e.g. rootScope.model = null, and let all the standard directives cascade. All other mechanisms detailed above leak (and badly)....

petebacondarwin commented 12 years ago

OK so first of all why would you want to destroy the rootscope? Destroying a scope simply removes it from the scope hierarchy so that it needs longer takes part in digests and if nothing references it it can be garbage collected. The destroy event propagates to all the scope's children so that directives can unbind event handlers and remove references to the scope. Pete

... sent from my tablet On 7 Nov 2012 14:50, "Daniel Luz" notifications@github.com wrote:

If you look at the $destroy() implementation, it only does two things:

  1. Broadcast $destroy on that scope
  2. Remove itself from its parent's and siblings' linked lists (no change to its children, they are just left for garbage collection)

Since the $rootScope has no parent or siblings, there's nothing to be done on the second step, so broadcasting the event should be enough. Granted, though, this is not the best future-proof approach.

— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/1537#issuecomment-10150567.

suedama1756 commented 12 years ago

What I want to do (as stated in the original question), is understand the correct way to tear down an angular based application. We have an application made up of "widgets" that are loaded dynamically (via require), each widget is a separate angular application (via manual bootstrap), we need to be able to un-install widgets. So, basically we need to tear down an angular application safely, without any leaks...

I "don't" expect $destroy to do this for me, and after looking at the code the other suggestions above seem naive (sorry guys).

petebacondarwin commented 12 years ago

Right. So you are manually bootstrapping a load of angular apps and you want to be able to remove them.

From looking at the code (rootScope and injector) it seems that all angular.bootstrap does is create an injector

var injector = createInjector(modules);

Then attach this injector to the DOM element data property

element.data('$injector', injector);

and finally compile up the element with the a new Scope, which is the rootScope.

compile(element)(scope);

At no point does anything get added to the global namespace. It is all attached to the root DOM Element. If this is the case, surely just deleting the element that was bootstrapped should be enough to release the injector. Have you found memory leaks when doing this?

If so then it is probably something for Misko, Igor or Vojta to take a look at.

Pete

On 7 November 2012 16:31, Jason Young notifications@github.com wrote:

What I want to do (as stated in the original question), is understand the correct way to tear down an angular based application. We have an application made up of "widgets" that are loaded dynamically (via require), each widget is a separate angular application (via manual bootstrap), we need to be able to un-install widgets. So, basically we need to tear down an angular application safely, without any leaks...

I "don't" expect $destroy to do this for me, and after looking at the code the other suggestions above seem naive (sorry guys).

— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/1537#issuecomment-10154693.

suedama1756 commented 12 years ago

Pete, thanks for looking into this for me, I do appreciate the efforts :) To be honest, I wasn't going to even post the issue, as I knew what the score was after looking at the code myself...(but I was convinced by a colleague that I might have missed something).

Anyway, we've still got leaks in our current implementation, even after striping things back, but only if we use certain angular features (so it seems). The one thing I need to do this morning is remove our "widget" framework completely from the picture and see where I am with a pure angular solution. Once I've done that I'll post my findings here for others, and if I track the issue I'll submit a pull request...

suedama1756 commented 12 years ago

OK, back to my original post here I think.

As angular doesn't appear to have an "official" mechanism for tear down, there is no way to tell the injector to dispose of all of its services, therefore, services such as browser which attach to window events have no idea that they should detach their event handlers, this leads indirectly to the root scope being held onto.

In our "widget" framework, when we destroy our context (which is similar to angular's injector), the context's lifetime container calls dispose on any services we are managing (if they support it), allowing them to perform cleanup.

This is what I believe is required to make angular support application tear down.

I'm going to do more investigation into this, but the retaining object tree in chromes memory profile is pretty conclusive...

I'll post further finds as I encounter them.

suedama1756 commented 12 years ago

The leak is fixed, although the solution is hacky at the moment.

I still believe an application tear down mechanism is required, however, this is a complicated issue which will require some thought.

Firstly, get the root of your application to use an ng-include (for applications that add/delete nested angular applications, this is probably your development model anyway), by setting the Url to null you can get angular to clean up pretty much everything for you.

In addition to this you need to then remove your application root node (calling cleanData is not enough) and get the browser object to unhook its events.

         injector.invoke(function($rootScope) {
            $rootScope.$apply(function() {
                 // get angular to tear down pretty much everything for us
                $rootScope.templateUrl = null;
            });
        });
        // hack to get browser object to unhook from global events
        injector.get('$browser').destroy();
        injector = null;

        var host = document.getElementById('applicationRoot');
        var parent = host.parentNode;
        angular.element(host).remove();
        angular.element(parent).append('<div id="applicationRoot"></div>');

Code added to browser object

  self.destroy = function () {
        if (urlChangeInit) {      // html5 history api - popstate event
            if ($sniffer.history) jqLite(window).unbind('popstate', fireUrlChange);
            // hashchange event
            if ($sniffer.hashchange) jqLite(window).unbind('hashchange', fireUrlChange);
        }
    }

As I say, not pretty, but for anyone else needing a temporary solution this could help....

vladgurovich commented 11 years ago

@suedama1756 I wonder how did your approach change based on this comment: https://groups.google.com/d/msg/angular/_AulBSlrspU/wE-rbQ2hl0AJ

jreading commented 11 years ago

I have the same issue bootstrapping ng apps in GWT and removing those elements from the DOM. App still exists in memory although angular.element('#theAppInQuestion').data() is empty after removal.

czechdude commented 10 years ago

I would like to vote up this issue, as for my app it is very important to make the angular application recyclable. So either allow reattaching the application scope to the same DOM element, or introduce some destroy method.

The 1.2 version of Angular disallows all the previously possible ways! It's a no-go!

Thanks for your consideration.

Narretz commented 10 years ago

has this been fixed by #5279 @petebacondarwin @czechdude @suedama1756 ?

petebacondarwin commented 10 years ago

@suedama1756 et al - can you confirm if #5279 (and other related leak fixes) solves your problems? If not, please comment pinging me.

I am going to close this issue for now. If you can provide a specific example of an issue that is not fixed by these changes then please create a new issue.

tkssharma commented 8 years ago

I am looking for same kind of solution want to clear scope data on tab close , i didn't find any solution to broadcast an event for $destroy scope

petebacondarwin commented 8 years ago

It is not clear what you want @tkssharma - you can listen to a $destroy event on a scope for when parent scopes are destroyed.

HeberLZ commented 8 years ago

Hi everyone, quick question kind of related to this, is there a way to either destroy a custom injector or use it as default during the bootstrapping process?

Basically i'm stopping angular's bootstrapping process in order to do some one-time only requests that i need before the app starts and then i resume it. Currently i'm doing those requests outside of angular, i thought about generating a manual injector in order to handle those requests through angular services but i don't like the idea of having two injectors once the app is finally bootstrapped.

gkalpak commented 8 years ago

@HeberLZ, there is currently no way to destroy an injetor. Maybe you can let it be garbage collected if you don't need it anymore. Essentially, you can create as many injectors as you want, using angular.injector(['module1', 'module2', ...]). When bootstraping an app, an injector is being created like this under the hood.

You just need to be careful when instantiating services that have "permanent" side-effects on the page (e.g. adding a listener on window or document etc). I think your best bet would be to call $rootScope.$destroy() on any injectors you don't need any more. E.g.:

function doSomethingBeforeBootstrapingMyApp() {
  // Create an injector
  var myTempInjector = angular.injector(['ng', 'someModule']);

  // Instantiate a service and use it
  var SomeService = myTempInjector.get('SomeService');
  SomeService.doSomething();

  // Clean up
  var $rootScope = myTempInjector.get('$rootScope');
  $rootScope.$destroy();
}

Btw, GitHub issues are reserved for bug reports and feature requests. You can use one of the general support channels for these types of questions.

ghost commented 4 years ago

ng g

Good day everyone and @petebacondarwin, sorry to bother everyone. I'm reading the documentation website. It does not have a documentation about this way of disposing the angular.bootstrap app.

How do I add the following documentation to the angularjs website: to dispose an angular.bootstrap(document.getElementById("my-div")), you simply have to remove that element document.getElementById("my-div"). This gets complicated if you want to destroy the angular.bootstrap with document.body. but i just need the one for removing the non-document.body element.

where should i add that from below? angular.bootstrap api - https://docs.angularjs.org/api/ng/function/angular.bootstrap bootstrap documentation - https://docs.angularjs.org/api/ng/function/angular.bootstrap compile documentation - https://docs.angularjs.org/guide/compiler

edit: assuming i will be following the design guidlines not using states outside the scope of each angular.bootstrap compiled elements. If i have many angular.bootstrap elements, their behaviours will be independent right? Meaning each injections and linkers does not share any state?

I will also add the last question to the documentation

petebacondarwin commented 4 years ago

to dispose an angular.bootstrap(document.getElementById("my-div")), you simply have to remove that element document.getElementById("my-div").

It has been a while since I have looked at this stuff, but I am not sure that simply removing the bootstrapped element is enough to actually clean up the application...

ghost commented 4 years ago

@petebacondarwin thank you for the response. sorry to bother you again. Is https://angular-ui.github.io/ under angularjs.org (the official ui class like https://material.angular.io/ for https://angular.io/) or a 3rd party team/project? how come it is not included or even mentioned in https://angularjs.org/ ?

petebacondarwin commented 4 years ago

AngularUI is a 3rd party project