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(animations): Ensure elements are removed from the cache after leave animation. #50929

Closed
wants to merge 1 commit into from
Closed

fix(animations): Ensure elements are removed from the cache after leave animation. #50929

wants to merge 1 commit into from

Conversation

JeanMeche
Copy link
Member

@JeanMeche JeanMeche commented Jul 3, 2023 •
edited

This commit fixes a memory leak where references were retained in multiple Map structures.

_namespaceLookup was cleared before the call to processLeaveNode() which was using the lookup.
Without that lookup clearElementCache() wasn't called thus keeping a reference to the element.

Fixes #24197 & #50533

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • No

@JeanMeche JeanMeche marked this pull request as ready for review July 3, 2023 23:22
@pullapprove pullapprove bot requested a review from jessicajaniuk July 3, 2023 23:22
@JeanMeche JeanMeche mentioned this pull request Jul 4, 2023
Closed
@pkozlowski-opensource pkozlowski-opensource added area: animations action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 5, 2023
@ngbot ngbot bot modified the milestone: Backlog Jul 5, 2023
JoostK
JoostK reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

Thanks for investigating, this must have been hard to track down!

packages/animations/browser/test/render/transition_animation_engine_spec.ts

expect(engine.elementContainsData(DEFAULT_NAMESPACE_ID, element)).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

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

This test definitely looked broken before, with both these elementContainsData calls being checked to return true even after removal and flush. I'd suggest also removing the first elementContainsData assertion as it seems rather meaningless with the new assertions, and it allows dropping test-only elementContainsData implementations from production code (both from TransitionAnimationsEngine and AnimationTransitionNamespace).

Copy link
Member Author

Choose a reason for hiding this comment

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

Great suggestion ! I'll remove it !

packages/animations/browser/src/render/transition_animation_engine.ts
this.afterFlushAnimationsDone(() => ns.destroy(context));
this.afterFlushAnimationsDone(() => {
ns.destroy(context);
delete this._namespaceLookup[namespaceId];
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit unfortunate to me that cleanup logic is (still) spread across two phases, especially now that we've seen that first-phase cleanup can remove needed state too early. Apparently the destroyed namespace is still interacting with this engine, so arguably we should not be cleaning it up until those interactions can no longer occur entirely?

tldr; can all of the afterFlush work be pushed into this second phase, or would that result in issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like, we're good with moving everything into afterFlushAnimationsDone !

@JeanMeche
fix(animations): Ensure elements are removed from the cache after lea…
f0e1011 
…ve animation.

This commit fixes a memory leak.

`_namespaceLookup` was cleared before the call to `processLeaveNode()` which was using the lookup.
Without that lookup `clearElementCache()` wasn't called thus keeping a reference to the element.

Fixes angular#24197 & angular#50533
jessicajaniuk
jessicajaniuk approved these changes Jul 10, 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.

Nice work fixing this!

@jessicajaniuk jessicajaniuk added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 10, 2023
@dylhunn
Copy link
Contributor

dylhunn commented Jul 11, 2023

This PR was merged into the repository by commit a14bdfe.

dylhunn pushed a commit that referenced this pull request Jul 11, 2023
@JeanMeche @dylhunn
fix(animations): Ensure elements are removed from the cache after lea…
f920fcb 
…ve animation. (#50929)

This commit fixes a memory leak.

`_namespaceLookup` was cleared before the call to `processLeaveNode()` which was using the lookup.
Without that lookup `clearElementCache()` wasn't called thus keeping a reference to the element.

Fixes #24197 & #50533

PR Close #50929
@dylhunn dylhunn closed this in a14bdfe Jul 11, 2023
@JeanMeche JeanMeche mentioned this pull request Jul 11, 2023
Closed
@JeanMeche JeanMeche deleted the fix/animations-leak branch July 12, 2023 08:06
sunilbaba pushed a commit to sunilbaba/angular that referenced this pull request Jul 26, 2023
@JeanMeche @sunilbaba
fix(animations): Ensure elements are removed from the cache after lea…
f61499b 
…ve animation. (angular#50929)

This commit fixes a memory leak.

`_namespaceLookup` was cleared before the call to `processLeaveNode()` which was using the lookup.
Without that lookup `clearElementCache()` wasn't called thus keeping a reference to the element.

Fixes angular#24197 & angular#50533

PR Close angular#50929
@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 Aug 12, 2023
thomasturrell pushed a commit to thomasturrell/angular that referenced this pull request Aug 29, 2023
@JeanMeche
fix(animations): Ensure elements are removed from the cache after lea…
944688f 
…ve animation. (angular#50929)

This commit fixes a memory leak.

`_namespaceLookup` was cleared before the call to `processLeaveNode()` which was using the lookup.
Without that lookup `clearElementCache()` wasn't called thus keeping a reference to the element.

Fixes angular#24197 & angular#50533

PR Close angular#50929
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@JeanMeche @ChellappanRajan
fix(animations): Ensure elements are removed from the cache after lea…
2b11f9c 
…ve animation. (angular#50929)

This commit fixes a memory leak.

`_namespaceLookup` was cleared before the call to `processLeaveNode()` which was using the lookup.
Without that lookup `clearElementCache()` wasn't called thus keeping a reference to the element.

Fixes angular#24197 & angular#50533

PR Close angular#50929
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoostK JoostK JoostK left review comments

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: animations 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.

Memory leak with animations in combination with @HostBinding
5 participants
@JeanMeche @dylhunn @JoostK @jessicajaniuk @pkozlowski-opensource