feat(shutdown): Add the ability for an app to shutdown #8005

lgalfaso · 2014-06-27T10:22:46Z

Adds a new $shutdown service that can be used to shutdown an app and the $shutdownProvider that can be used to register tasks that need to be executed when shutting down an app.

Added tasks for $rootElement, $rootScope and $browser to be able to shutdown an app

Refactor $interval so it delegates to $browser just like $timeout does

mary-poppins · 2014-06-27T10:22:49Z

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

Uses the issue template (#8005)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

Narretz · 2014-06-27T12:20:34Z

test/AngularSpec.js

+      expect(element.text()).toBe('Bootstrap me!');
+    });
+
+    it('should throw if trying to bootstrap an element that is not part of an app', function() {


This should be "to shutdown"

IgorMinar · 2014-07-22T17:53:53Z

src/Angular.js

+function rootElementProviderFactory(rootElement) {
+  return ['$shutdownProvider', function($shutdownProvider) {
+    $shutdownProvider.register(function() {
+      if (rootElement.dealoc) rootElement.dealoc();


is this jQuery specific?

rootElement.dealoc() is the jqLite specific way to recursively remove the data from an element.

This api was removed from master recently in 9c5b407.

so the truthy branch should be removed. removeData() will do the job.

There is no way to say "recursively remove all the data" (well, not without manually traversing the tree). In jQuery this is possible doing rootElement.find('*').removeData(), but not with jqLite.
Will write a manual traversing and repost the PR.

JohannUlbrich · 2015-07-14T07:15:46Z

The possibility to shutdown an Angular app is still a big issue for me. Are there any plans to include this in Angular 1.4?

gkalpak · 2016-06-01T12:51:16Z

@lgalfaso, what is the status of this ?

gkalpak · 2016-06-01T12:56:22Z

Fixes #7578

lgalfaso · 2016-06-01T14:02:41Z

This is quite old but the approach still valid. Can work on getting this up
to date so it can land of the need is still needed

On Wednesday, June 1, 2016, Georgios Kalpakas [email protected]
wrote:

@lgalfaso https://github.com/lgalfaso, what is the status of this ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8005 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAX44oIWPieqwtvK7iwNFOZtdg_Q2treks5qHYBagaJpZM4CIKVw
.

lgalfaso · 2016-06-03T18:24:38Z

Updated the PR. @gkalpak do you want to take a look at it?

gkalpak · 2016-06-23T20:26:45Z

src/Angular.js

+      if (rootElement.dealoc) {
+        rootElement.dealoc();
+      } else {
+        rootElement.find('*').removeData();


Not a common case for sure, but this would also "clean" inside [ng-non-bindable] element (where theoretically could leave other angular apps).

if there is an outer and an inner app, and the outer app is shutdown (remember that this will trigger a $destroy event on $rootScope), I would expect that it is reasonable for the inner app to also get destroyed.

No, it shouldn't. These are independent apps, so destroying the outer, shouldn't destroy the inner. To be clear, this is what I am talking about:

<body app1> Hello from {{ 'app 1' }}! <div ng-non-bindable> <div app2> Hello from {{ 'app 2' }}! </div> </div> </body>

angular.module('app1', []); angular.module('app2', []); angular.bootstrap(document.querySelector('[app1]'), ['app1']); angular.bootstrap(document.querySelector('[app2]'), ['app2']);

Well, this is currently not possible, as in cases like this we do not keep track of what elements belong to app1 and what elements belong to app2

It is possible to ignore anything under ng-non-bindable (because we haven't touched it anyway).

Maybe something like this (which is not super efficient, but shouldn't really matter, when shutting down):

var unvisitedNodes = [$rootElement[0]]; var foundNodes = [$rootElement[0]]; var node; while ((node = unvisitedNodes.pop())) { var newNodes = Array.prototype.filter.call(node.children, function (child) { return directiveNormalize(nodeName_(child)) !== 'ngNonBindable'; }); Array.prototype.push.apply(unvisitedNodes, newNodes); Array.prototype.push.apply(foundNodes, newNodes); } jqLite.cleanData(foundNodes);

@gkalpak if we want to support nested ng-apps in ng-non-bindable containers perhaps we should add at least basic tests verifying that it works?

Perhaps. But basic tests wouldn't help much (I mean even if basic tests worked, some less basic code could still be broken). The question is whether we want to break this usecase or not (I still wouldn't).

Adds a new `$shutdown` service that can be used to shutdown an app and the `$shutdownProvider` that can be used to register tasks that need to be executed when shutting down an app

lgalfaso · 2016-06-24T10:02:01Z

@gkalpak PTAL

gkalpak · 2016-06-25T11:41:35Z

src/ng/rootScope.js

@@ -907,8 +911,7 @@ function $RootScopeProvider() {
        this.$$destroyed = true;

        if (this === $rootScope) {
-          //Remove handlers attached to window when $rootScope is removed
-          $browser.$$applicationDestroyed();
+          $shutdown();


I am not sure we need this, now that $shutdown is a public service.

@gkalpak What do you mean?

Well, it's been a while, but what I think I meant that this was a poor man's $shutdown(). Before $shutdown() was implemented, we were special-casing $rootScope.$destroy() to do destroy the application (because $browser.$$applicationDestroyed() was private). But now that $shutdown() is a public service, people wanting to destroy the app can (and should) use that (instead of destroying $rootScope.

It doesn't seem too harmful though, so we might as well keep it to avoid the breaking change.

gkalpak · 2016-06-25T12:07:57Z

Generally LGTM!

I have left a couple minor comments (mainly related to cleaning up $rootElement).
I think there are also a couple more things to clean up (or take care of in a way that they will get cleaned up automatically):

Cancel (or at least ignore when completed) pending XHR/JSONP requests.
Clean up JSONP callbacks (stored on angular.windows.callbacks)[*].
Clean up listeners added with addEventListener on form/ngForm elements.
Clean up listeners on $document (e.g. .on('visibilitychange')).

*: I just realized that bootstrapping multiple apps on the same page, could mess up the JSONP callbacks, since each app will generate the same callback IDs.

lgalfaso · 2016-06-28T18:17:17Z

Cancel (or at least ignore when completed) pending XHR/JSONP requests.

I think that if we ignore $digest on destroyed scopes (I know this is a BC), this should be enough

Clean up JSONP callbacks (stored on angular.windows.callbacks)[*].

Oh, another part that we do not keep track from what app each callback comes... anyhow the same solution to ignore $digest should work

Clean up listeners added with addEventListener on form/ngForm elements.

We should be fine here as we trigger $destroy

Clean up listeners on $document (e.g. .on('visibilitychange')).

We should be fine here as we trigger $destroy

gkalpak · 2016-06-28T21:39:24Z

Cancel (or at least ignore when completed) pending XHR/JSONP requests.

I think that if we ignore $digest on destroyed scopes (I know this is a BC), this should be enough

We already keep track of the pending requests in $http (although only the configs). How about we start keeping track of active XHRs instances as well and have $http register with $shutdownProvider to abort them on shut-down?

Clean up JSONP callbacks (stored on angular.windows.callbacks)[*].

Oh, another part that we do not keep track from what app each callback comes... anyhow the same solution to ignore $digest should work

Now that we have a $jsonpCallbacks service, we could keep track of the added callback IDs and have $jsonpCallbacks delete them on shut-down.

Clean up listeners added with addEventListener on form/ngForm elements.

We should be fine here as we trigger $destroy

Good point. Missed that 😃

Clean up listeners on $document (e.g. .on('visibilitychange')).

We should be fine here as we trigger $destroy

Are we calling $destroy on $document?

mgol · 2017-03-29T11:11:53Z

src/Angular.js

+        rootElement.dealoc();
+      } else {
+        rootElement.find('*').removeData();
+        rootElement.removeData();


This is not enough as it won't remove event listeners. How about:

jqLite.cleanData(rootElement.find('*')); jqLite.cleanData(rootElement);

or something analogous to @gkalpak's code but using cleanData if we want to not break ng-non-bindable-contained AngularJS apps.

This, BTW, made me notice that jqLite's implementation of removeData is really broken. It should only remove user-set data and not touch the event handlers storage at all. I reported #15869 to fix that in 1.7.

mgol · 2017-03-29T11:19:16Z

src/ng/browser.js

+      }, delay || 0);
+      pendingDeferIds[timeoutId] = true;
+    } else {
+      timeoutId = 0;


What's the purpose of this line?

mgol · 2017-03-29T11:27:54Z

src/ngMock/angular-mocks.js

+    });
+    self.interval.repeatFns.sort(function(a,b) { return a.nextTime - b.nextTime;});
+
+    return self.interval.nextRepeatId++;


Please separate this into two instructions; ease of reading is more important than being shorter, especially in mocks code.

mgol · 2017-03-29T11:30:14Z

src/ngMock/angular-mocks.js

+      var task = self.interval.repeatFns[0];
+      task.fn();
+      task.nextTime += task.delay;
+      self.interval.repeatFns.sort(function(a,b) { return a.nextTime - b.nextTime;});


Is this necessary? The queue is sorted during callback registration already.

mgol · 2017-03-29T11:32:54Z

test/AngularSpec.js

@@ -1671,6 +1671,110 @@ describe('angular', function() {
    });
  });

+  describe('shutdown', function() {


We should test that event handlers are removed as well (current code doesn't do that when jQuery is used).

mgol · 2018-06-20T20:00:38Z

This is a pretty big change which has a potential of introducing regressions. Considering that and taking into account the fact that we're getting close to the LTS phase when only a few kinds of issues get fixed (like security ones) we've decided to not pursue it further.

lgalfaso added cla: yes and removed cla: no labels Jun 27, 2014

Narretz reviewed Jun 27, 2014
View reviewed changes

Narretz added the type: feature label Jul 1, 2014

btford added the component: misc core label Jul 8, 2014

Narretz added this to the Backlog milestone Jul 16, 2014

IgorMinar reviewed Jul 22, 2014
View reviewed changes

IgorMinar mentioned this pull request Jul 22, 2014

fix($rootScope): cleanup on $destroy also for $rootScope #7564

Closed

This was referenced Jul 22, 2014

ngRoute not properly destructed upon $destroy of $rootScope #8295

Closed

Destroying an angular app #7578

Open

btford removed the gh: PR label Aug 20, 2014

caitp force-pushed the master branch from 591bac5 to d713ad1 Compare August 22, 2014 13:51

tbosch force-pushed the master branch from 6c27b89 to 271572c Compare August 30, 2014 00:57

tbosch force-pushed the master branch from a3d7934 to 1418383 Compare September 10, 2014 18:14

petebacondarwin force-pushed the master branch from 02dc2aa to fd2d6c0 Compare September 16, 2014 11:26

jeffbcross force-pushed the master branch 2 times, most recently from cad9560 to f294244 Compare October 2, 2014 22:09

btford force-pushed the master branch from 9011a65 to 46db47b Compare October 6, 2014 22:54

jeffbcross force-pushed the master branch 4 times, most recently from e8dc429 to e83fab9 Compare October 10, 2014 17:37

petebacondarwin force-pushed the master branch from 4dd5a20 to 998c61c Compare October 19, 2014 10:57

lgalfaso mentioned this pull request Nov 4, 2014

fix($rootScope): remove history event handler when app is torn down #9905

Closed

lgalfaso mentioned this pull request Jan 20, 2015

Application Shutdown Event #10817

Closed

petebacondarwin mentioned this pull request Feb 2, 2015

fix($rootScope): allow destroying a root scope #10895

Closed

Narretz force-pushed the master branch from 70049ae to b77e14b Compare January 16, 2016 21:59

lgalfaso force-pushed the shutdown branch 2 times, most recently from ae83ce2 to f77e965 Compare June 3, 2016 18:20

lgalfaso force-pushed the shutdown branch from f77e965 to aa2f015 Compare June 3, 2016 18:40

gkalpak reviewed Jun 23, 2016
View reviewed changes

feat(shutdown): Add the ability for an app to shutdown

4e9b568

Adds a new `$shutdown` service that can be used to shutdown an app and the `$shutdownProvider` that can be used to register tasks that need to be executed when shutting down an app

lgalfaso force-pushed the shutdown branch from aa2f015 to 4e9b568 Compare June 24, 2016 10:01

gkalpak reviewed Jun 25, 2016
View reviewed changes

gkalpak mentioned this pull request Oct 10, 2016

Bug: Memory leak on removing app's root element #15239

Closed

mgol suggested changes Mar 29, 2017

View reviewed changes

mgol closed this Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(shutdown): Add the ability for an app to shutdown #8005

feat(shutdown): Add the ability for an app to shutdown #8005

lgalfaso commented Jun 27, 2014

mary-poppins commented Jun 27, 2014

Narretz Jun 27, 2014

IgorMinar Jul 22, 2014

lgalfaso Jul 22, 2014

IgorMinar Jul 22, 2014

lgalfaso Jul 22, 2014

JohannUlbrich commented Jul 14, 2015

gkalpak commented Jun 1, 2016

gkalpak commented Jun 1, 2016

lgalfaso commented Jun 1, 2016

lgalfaso commented Jun 3, 2016

gkalpak Jun 23, 2016

lgalfaso Jun 24, 2016

gkalpak Jun 25, 2016

lgalfaso Jun 28, 2016

gkalpak Jun 28, 2016

gkalpak Jun 28, 2016

mgol Mar 29, 2017

gkalpak Mar 29, 2017

lgalfaso commented Jun 24, 2016

gkalpak Jun 25, 2016

mgol Mar 29, 2017

gkalpak Sep 18, 2017

gkalpak commented Jun 25, 2016

lgalfaso commented Jun 28, 2016

gkalpak commented Jun 28, 2016

mgol Mar 29, 2017

mgol Mar 29, 2017

mgol Mar 29, 2017

mgol Mar 29, 2017

mgol Mar 29, 2017

mgol commented Jun 20, 2018

feat(shutdown): Add the ability for an app to shutdown #8005

feat(shutdown): Add the ability for an app to shutdown #8005

Conversation

lgalfaso commented Jun 27, 2014

mary-poppins commented Jun 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohannUlbrich commented Jul 14, 2015

gkalpak commented Jun 1, 2016

gkalpak commented Jun 1, 2016

lgalfaso commented Jun 1, 2016

lgalfaso commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgalfaso commented Jun 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jun 25, 2016

lgalfaso commented Jun 28, 2016

gkalpak commented Jun 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Jun 20, 2018