$destroy on rootScope is ignored #1537

Closed
suedama1756 opened this Issue Nov 7, 2012 · 20 comments

Comments

Projects
None yet
10 participants

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...

Member

petebacondarwin commented Nov 7, 2012

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

Contributor

mernen commented Nov 7, 2012

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.

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();
});
Contributor

mernen commented Nov 7, 2012

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.

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)....

Member

petebacondarwin commented Nov 7, 2012

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.

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).

Member

petebacondarwin commented Nov 7, 2012

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.

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...

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.

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....

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

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.

@Downchuck Downchuck referenced this issue in angular-ui/ui-grid Nov 17, 2013

Closed

Document cleanup/destroy semantics #833

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.

Contributor

Narretz commented Dec 18, 2013

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

Member

petebacondarwin commented Dec 19, 2013

@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.

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

Member

petebacondarwin commented Jan 25, 2016

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

Contributor

HeberLZ commented Jul 20, 2016

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.

Member

gkalpak commented Jul 20, 2016

@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.

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