fix(core): prevent dangling prevConsumer reference from leaking v2#68681
Open
milomg wants to merge 3 commits into
Open
fix(core): prevent dangling prevConsumer reference from leaking v2#68681milomg wants to merge 3 commits into
milomg wants to merge 3 commits into
Conversation
…oyed views When `producerAccessed` creates a new link for a non-live consumer (e.g. a computed signal with no readers), it eagerly sets `prevConsumer` to the producer's current `consumersTail`. However, because the consumer is not live, `producerAddLiveConsumer` is skipped and the link is never inserted into the producer's consumer doubly-linked list. This means the link holds a reference *into* the producer's consumer list without being *part* of it. When the node that `prevConsumer` points to is later removed via `producerRemoveLiveConsumerLink`, the dangling link is not patched because it isn't traversable from the list. The result is that the removed consumer link — and everything it references — is kept alive by the dangling `prevConsumer` pointer on the non-live link, which itself is kept alive through the computed signal's `producers` linked list. In practice this causes multi-MB memory leaks in Angular apps: a root-provided service with a computed signal (e.g. `AttachmentApiService.urls`) holds a producer link to `ApplicationEnvironmentService.environmentSignal`. That link's `prevConsumer` captures a stale reference to a destroyed view's `ReactiveLViewConsumer` link, retaining the entire LView hierarchy — components, QueryLists, ElementRefs, and detached DOM — after the view is destroyed. The fix initializes `prevConsumer` to `undefined` at link creation time. This is safe because `producerAddLiveConsumer` unconditionally sets `link.prevConsumer = consumersTail` (line 513) when the link is actually inserted into the consumer list. The value set in `producerAccessed` was always overwritten for live consumers, and was never correct for non-live consumers. Made-with: Cursor
Uses WeakRef + global.gc() to verify that destroyed effect consumers become garbage-collectable when a non-live computed reads the same producer. The jasmine_test target is configured with node_options: --expose-gc. GC tests are skipped in browser targets via isBrowser from @angular/private/testing. Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The current signals implementation leaks
What is the new behavior?
See the analysis in #68137, this is just a new PR so that I can add additional edits to the test implementation
Does this PR introduce a breaking change?
Other information