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

Factory provider type does not respect OnDestroy hook #22240

Closed
tmair opened this issue Feb 15, 2018 · 5 comments
Closed

Factory provider type does not respect OnDestroy hook #22240

tmair opened this issue Feb 15, 2018 · 5 comments

Comments

@tmair
Copy link

@tmair tmair commented Feb 15, 2018

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report 
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

If we provide a service with a ngOnDestroy method as a type provider the service's ngOnDestroy lifecycle hook is called. If however the same service is provided via a factory function, the ngOnDestroy lifecycle hook is never called.

Expected behavior

The behavior should not differ if using a type provider or a factory provider.

Minimal reproduction of the problem with instructions

Simple Demo: https://stackblitz.com/edit/angular-3azkmn

If you comment in the type provider, the ngOnDestroy hook is called (console.log is performed).
If you use the factory provider the ngOnDestroy hook is not called

What is the motivation / use case for changing the behavior?

I have tried to destroy an angular application manually (by calling the NgModuleRef.destroy method) that contains an angular router. After the appliction is destroyed the routing events still fire. The cause for this is that the ngOnDestroy hook is never called. Further this leads to a memory leak, as the application is not garbage collected.

Other issues might be that someone changes the provider type and is not aware of this limitation. Thus introducing subtle bugs into the appliction.

Solution

I do not exactly know where to look for a fix, but the nodeDef.flags do not contain the OnDestroy bit. So the cause of this bug is probably somwhere in the compiler, which I am not too familiar with.

Environment


Angular version: ^5.2.0

Browser:
- [x] Chrome (desktop) version XX
- [x] Chrome (Android) version XX
- [x] Chrome (iOS) version XX
- [x] Firefox version XX
- [x] Safari (desktop) version XX
- [x] Safari (iOS) version XX
- [x] IE version XX
- [x] Edge version XX
@ngbot ngbot bot added this to the Backlog milestone Feb 15, 2018
@vicb
Copy link
Contributor

@vicb vicb commented Feb 15, 2018

For now this works as intended as we check the presence of OnDestroy at compile time.

@tmair
Copy link
Author

@tmair tmair commented Feb 15, 2018

@vicb What about the issue with the router? The router class implements the OnDestroy interface but because it is provided via a factory the destroy hook is never called. Should I file a separate issue for that?

@trotyl
Copy link
Contributor

@trotyl trotyl commented Feb 18, 2018

Duplicate of #14818. Better to file a separate issue for router memory leak problem.

alxhub added a commit to alxhub/angular that referenced this issue 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 angular#22466
Fixes angular#22240
Fixes angular#14818
alxhub added a commit to alxhub/angular that referenced this issue 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 angular#22466
Fixes angular#22240
Fixes angular#14818
@IgorMinar IgorMinar closed this in fc03427 May 8, 2018
IgorMinar added a commit that referenced this issue May 8, 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
Fixes #22240
Fixes #14818

PR Close #23755
@simeyla
Copy link

@simeyla simeyla commented Aug 29, 2019

@vicb this also doesn't work with an InjectionToken which surprised me very much. Surely that can be figured out at compile time?

    {
        provide: PAYMENTPANEL_SERVICE,
        useClass: ShopPaymentPanelService           // useClass
    }   

ngOnDestroy is never called on the above - despite using useClass

If I switch this to the following then my ngOnDestroy does get called.

    [ 
     ShopPaymentPanelService,
     {
              provide: PAYMENTPANEL_SERVICE,
              useExisting: ShopPaymentPanelService     // useExisting
     }   
    ]

I can see how it's much more complicated with a factory - but surely this should work?

@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Sep 29, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.