Skip to content

fix(core): onDestroy should be registered only on valid DestroyRef#49804

Closed
pkozlowski-opensource wants to merge 2 commits intoangular:mainfrom
pkozlowski-opensource:destroy_ref_destroyed
Closed

fix(core): onDestroy should be registered only on valid DestroyRef#49804
pkozlowski-opensource wants to merge 2 commits intoangular:mainfrom
pkozlowski-opensource:destroy_ref_destroyed

Conversation

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Apr 12, 2023

It might happen that the lifecycle scope represented by DestroyRef becomes invalid before an onDestroy hook is registered (ex. injector or component instance got destroyed). In such cases registration of the onDestroy hooks should no longer be possible.

Fixes #49658

@pkozlowski-opensource pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: rc This PR is targeted for the next release-candidate labels Apr 12, 2023
@ngbot ngbot bot added this to the Backlog milestone Apr 12, 2023
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review April 12, 2023 09:07
@pkozlowski-opensource pkozlowski-opensource mentioned this pull request Apr 12, 2023
Closed
@pkozlowski-opensource
Copy link
Member Author

pkozlowski-opensource commented Apr 12, 2023

De-flaked, green TGP

@yjaaidi
Copy link
Contributor

yjaaidi commented Apr 12, 2023 •
edited
Loading

@pkozlowski-opensource shouldn't we at least log a warning when not in dev mode?
As one of the situations where this could happen is race conditions, users might miss this when developing but only notice it on production builds.

I think that users will prefer a warning log message to a potential leak.

alxhub
alxhub requested changes Apr 13, 2023
View reviewed changes
@alxhub alxhub modified the milestones: v16-candidates, v16-final Apr 13, 2023
@pkozlowski-opensource
fix(core): onDestroy should be registered only on valid DestroyRef
f975de0 
It might happen that the lifecycle scope represented by DestroyRef becomes
invalid before an onDestroy hook is registered (ex. injector or component
instance got destroyed). In such cases registration of the onDestroy hooks
should no longer be possible.

Fixes angular#49658
@AndrewKushnir AndrewKushnir requested a review from alxhub April 13, 2023 20:36
jessicajaniuk
jessicajaniuk approved these changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from AndrewKushnir April 13, 2023 20:44
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Apr 13, 2023

FYI, I've also pushed a fixup commit with the golden file update to include a new error code. There were no functional changes in the fixup commit.

AndrewKushnir
AndrewKushnir approved these changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 13, 2023
@jessicajaniuk jessicajaniuk added the action: merge The PR is ready for merge by the caretaker label Apr 13, 2023
@jessicajaniuk jessicajaniuk added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Apr 13, 2023
@jessicajaniuk
Copy link
Contributor

jessicajaniuk commented Apr 13, 2023

Caretaker: This is ready for merge. TGP was green.

@jessicajaniuk
Copy link
Contributor

jessicajaniuk commented Apr 13, 2023

This PR was merged into the repository by commit 2c22e6f.

jessicajaniuk pushed a commit that referenced this pull request Apr 13, 2023
@pkozlowski-opensource @thePunderWoman
fix(core): onDestroy should be registered only on valid DestroyRef (#…
c34d7e0 
…49804)

It might happen that the lifecycle scope represented by DestroyRef becomes
invalid before an onDestroy hook is registered (ex. injector or component
instance got destroyed). In such cases registration of the onDestroy hooks
should no longer be possible.

Fixes #49658

PR Close #49804
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented May 18, 2023

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 May 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alxhub alxhub alxhub approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

+2 more reviewers

@yjaaidi yjaaidi yjaaidi left review comments

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: rc This PR is targeted for the next release-candidate

Projects

None yet

Milestone

v16-final

Development

Successfully merging this pull request may close these issues.

takeUntilDestroyed might not work as expected in certain situations

5 participants

@pkozlowski-opensource @yjaaidi @AndrewKushnir @jessicajaniuk @alxhub