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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

pkozlowski-opensource
Copy link
Member

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

De-flaked, green TGP

@yjaaidi
Copy link
Contributor

yjaaidi commented Apr 12, 2023 •
edited

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

@AndrewKushnir
Copy link
Contributor

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

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

@jessicajaniuk
Copy link
Contributor

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

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

@yjaaidi yjaaidi yjaaidi left review comments

@alxhub alxhub alxhub approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

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