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

fix(core): call ngOnDestroy on all services that have it #23755

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@alxhub
Contributor

alxhub commented May 7, 2018

Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes #22466

@googlebot googlebot added the cla: yes label May 7, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented May 7, 2018

@alxhub alxhub requested a review from vicb May 7, 2018

@alxhub alxhub force-pushed the alxhub:on-destroy branch from 087701c to a8af547 May 7, 2018

@@ -150,6 +150,11 @@ function _createProviderInstance(ngModule: NgModuleData, providerDef: NgModulePr
injectable = providerDef.value;
break;
}
if (injectable !== UNDEFINED_VALUE && injectable !== null && typeof injectable === 'object' &&
!(providerDef.flags & NodeFlags.OnDestroy) && typeof injectable.ngOnDestroy === 'function') {

This comment has been minimized.

@vicb

vicb May 7, 2018

Contributor

Those tests do not look needed: typeof injectable === 'object' && !(providerDef.flags & NodeFlags.OnDestroy)

This comment has been minimized.

@alxhub

alxhub May 7, 2018

Contributor

The goal of these two checks is to avoid a megamorphic read of ngOnDestroy if there's no chance it will be there (not an object) or if we've already statically detected it there (OnDestroy flag set).

This comment has been minimized.

@alxhub

alxhub May 7, 2018

Contributor

I will update this so undefined is handled properly.

for (let i = 0; i < def.providers.length; i++) {
const provDef = def.providers[i];
if (provDef.flags & NodeFlags.OnDestroy) {
const instance = ngModule._providers[i];
if (instance && instance !== UNDEFINED_VALUE) {
instance.ngOnDestroy();
const onDestroy: Function|undefined = instance.ngOnDestroy;

This comment has been minimized.

@vicb

vicb May 7, 2018

Contributor

not sure why:

  • you re-test the presence of ngOnDestroy here ?
  • you have this Set()

Looks like there is some overlap with changes in ng_module.ts

This comment has been minimized.

@alxhub

alxhub May 7, 2018

Contributor

I don't understand what you mean by overlap - this is the same file.

We re-test because it's not a guarantee that every call to a factory function will return the same shape object. The flags are shared across all instances of the module, so if the factory ever returns an instance with ngOnDestroy it'll get set, even if other instances it returned previously don't have one. So we need to check the individual instances before we call it.

The Set's job is to guarantee that we don't call ngOnDestroy twice on the same instance in the same injector. Previously this wasn't possible as only useClass providers would call onDestroy, and useClass providers always get their own instance. Now, useFactory and useValue can also give back instances, and could possibly give the same instance for multiple tokens. Therefore it's necessary to track instances and only call ngOnDestroy if it hasn't been called before on the same instance.

Theoretically it's possible that the user could provide the same instance in different injectors, and it would be disposed once per injector, but I think that's reasonable behavior for an edge case.

@mary-poppins

This comment has been minimized.

mary-poppins commented May 7, 2018

@vicb

This comment has been minimized.

Contributor

vicb commented May 7, 2018

Could we minize runtime perf impact by checking for the method only in the case where we can not determine it statically - ie not when provided as a class ?

@mhevery

mhevery approved these changes May 7, 2018

@mhevery

This comment has been minimized.

Member

mhevery commented May 7, 2018

Can you update the commit message to also close #14818, #22240 as they are duplicates.

@alxhub alxhub force-pushed the alxhub:on-destroy branch from a8af547 to a9b31ea May 7, 2018

@alxhub

This comment has been minimized.

Contributor

alxhub commented May 7, 2018

Could we minize runtime perf impact by checking for the method only in the case where we can not determine it statically - ie not when provided as a class ?

We do - the check for !(providerDef.flags & NodeFlags.OnDestroy) does this.

@mary-poppins

This comment has been minimized.

mary-poppins commented May 7, 2018

fix(core): call ngOnDestroy on all services that have it
Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes #22466
Fixes #22240
Fixes #14818

@alxhub alxhub force-pushed the alxhub:on-destroy branch from a9b31ea to 97b3aa6 May 7, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented May 7, 2018

@IgorMinar IgorMinar closed this in fc03427 May 8, 2018

IgorMinar added a commit that referenced this pull request May 8, 2018

fix(core): call ngOnDestroy on all services that have it (#23755)
Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes #22466
Fixes #22240
Fixes #14818

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