fix(core): cleanup `_ejsa` when app is destroyed by arturovt · Pull Request #59492 · angular/angular · GitHub
Skip to content

fix(core): cleanup _ejsa when app is destroyed - #59492

Closed
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/core-add-remove-event-info-method
Closed

fix(core): cleanup _ejsa when app is destroyed#59492
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/core-add-remove-event-info-method

Conversation

@arturovt

@arturovt arturovt commented Jan 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

In this commit, we delete _ejsa when the app is destroyed, ensuring that no elements are still captured in the global list and are not prevented from being garbage collected.

@angular-robot angular-robot Bot added the area: core Issues related to the framework runtime label Jan 12, 2025
@ngbot ngbot Bot added this to the Backlog milestone Jan 12, 2025
@tbondwilkinson

tbondwilkinson commented Jan 14, 2025

Copy link
Copy Markdown
Contributor

@arturovt

arturovt commented Jan 14, 2025

Copy link
Copy Markdown
Contributor Author

Generally speaking the EarlyJsactionData is cleared as soon as its used, so there would be no references to q left. It's unclear where you're seeing a memory leak.

#59261

@tbondwilkinson

tbondwilkinson commented Jan 14, 2025

Copy link
Copy Markdown
Contributor

_ejsa should be deleted when the app is destroyed - that's the proper fix for this, I suspect

@arturovt
arturovt force-pushed the fix/core-add-remove-event-info-method branch from c8ca84b to 301d77a Compare January 14, 2025 21:06
@arturovt arturovt changed the title fix(core): add remove event info handler fix(core): cleanup _ejsa when app is destroyed Jan 14, 2025
@arturovt arturovt changed the title fix(core): cleanup _ejsa when app is destroyed fix(core): cleanup _ejsa when app is destroyed Jan 14, 2025
@arturovt
arturovt force-pushed the fix/core-add-remove-event-info-method branch from 301d77a to 9c72e21 Compare January 14, 2025 21:07
@thePunderWoman

thePunderWoman commented Jan 14, 2025

Copy link
Copy Markdown
Contributor

@arturovt Can you add tests for this so we don't regress in the future?

tbondwilkinson
tbondwilkinson reviewed Jan 14, 2025
View reviewed changes
Comment thread packages/core/src/hydration/event_replay.ts Outdated
Comment thread packages/core/src/hydration/event_replay.ts Outdated

@tbondwilkinson tbondwilkinson Jan 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe the method that's used below is:

clearAppScopedEarlyEventContract(appId);

@arturovt arturovt Jan 14, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reused that function.

@arturovt
arturovt force-pushed the fix/core-add-remove-event-info-method branch from 9c72e21 to beaad8d Compare January 14, 2025 21:36
@arturovt
fix(core): cleanup _ejsa when app is destroyed
08437e5 
In this commit, we delete `_ejsa` when the app is destroyed, ensuring that no elements are still captured in the global list and are not prevented from being garbage collected.
@arturovt
arturovt force-pushed the fix/core-add-remove-event-info-method branch from beaad8d to 08437e5 Compare January 14, 2025 21:37
@arturovt
arturovt marked this pull request as ready for review January 15, 2025 08:58
@pullapprove
pullapprove Bot requested a review from thePunderWoman January 15, 2025 08:58
thePunderWoman
thePunderWoman approved these changes Jan 15, 2025
View reviewed changes

@thePunderWoman thePunderWoman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thank you!

@thePunderWoman thePunderWoman added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels Jan 16, 2025
@AndrewKushnir

AndrewKushnir commented Jan 16, 2025

Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit f6e9776.

The changes were merged into the following branches: main, 19.1.x

AndrewKushnir pushed a commit that referenced this pull request Jan 16, 2025
@arturovt @AndrewKushnir
fix(core): cleanup _ejsa when app is destroyed (#59492)
4eb5418 
In this commit, we delete `_ejsa` when the app is destroyed, ensuring that no elements are still captured in the global list and are not prevented from being garbage collected.

PR Close #59492
@arturovt
arturovt deleted the fix/core-add-remove-event-info-method branch January 16, 2025 22:15
PrajaktaB27 pushed a commit to PrajaktaB27/angular that referenced this pull request Feb 7, 2025
@arturovt @PrajaktaB27
fix(core): cleanup _ejsa when app is destroyed (angular#59492)
0304e8f 
In this commit, we delete `_ejsa` when the app is destroyed, ensuring that no elements are still captured in the global list and are not prevented from being garbage collected.

PR Close angular#59492
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Feb 16, 2025

Copy link
Copy Markdown

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Feb 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@thePunderWoman thePunderWoman thePunderWoman approved these changes
@tbondwilkinson tbondwilkinson tbondwilkinson 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 target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@arturovt @tbondwilkinson @thePunderWoman @AndrewKushnir