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.

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

fix(core): use WeakRef to prevent object retention in WeakMap #55476

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Apr 23, 2024

Although keys are not strongly referenced in a WeakMap, values are, potentially leading to data retention issues and improper garbage collection. By utilizing WeakRef, this problem can be mitigated effectively. Weak references allow the garbage collector to collect an object even if it is only weakly referenced. This can prevent memory leaks in the injector debugger profiler.

Closes #55396

@alan-agius4 alan-agius4 added area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Apr 23, 2024
@ngbot ngbot bot modified the milestone: Backlog Apr 23, 2024
@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Apr 23, 2024

Caretaker note: this requires cl/627330384 to be landed in G3

@alan-agius4 alan-agius4 marked this pull request as ready for review April 23, 2024 10:35
@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Apr 23, 2024
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: fw-security, dev-infra

@alan-agius4 alan-agius4 removed this from the Backlog milestone Apr 23, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 23, 2024
@alan-agius4
Copy link
Contributor Author

TGP

Comment on lines +58 to +60
resolverToTokenToDependencies = new WeakMap<
Injector|LView, WeakMap<Type<unknown>|InjectionToken<unknown>, WeakRef<InjectedService[]>>>();
resolverToProviders = new WeakMap<Injector|TNode, WeakRef<ProviderRecord[]>>();
Copy link
Member

Choose a reason for hiding this comment

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

How would this work, given that the WeakRef holds an array that is uniquely referenced in the WeakMap? Wouldn't each individual array item need to be a WeakRef—or alternatively replace the array with a WeakSet (that probably won't work as I assume iteration is necessary)—to avoid collecting the array entirely since the WeakMap is the sole owner of the array otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a WeakRef for each individual item would also work, however it seems redundant.

From my understanding is that the leak was being because whilst the WeakMap doesn’t hold keys as strong reference, the value are, setting the containing array in a WeakRef seems to solve this, in fact in the profiler the leak was no longer observed.

Are you thinking that the value WeakRef might be collected before the WeakMap key?

Copy link
Member

Choose a reason for hiding this comment

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

My reasoning is that the array that is stored in the WeakRef is the only place it's stored, so the only retainer is the WeakRef itself which is a weak retainer... so I'd presume that the array is always eligible to be GC'd. That's my mental model though, my understanding might certainly be inaccurate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what I though at first too but it does not appear to be the case.

const wm = new WeakMap();
const key = { a: 1 };
wm.set(key, new WeakRef([2]));

globalThis.gc();

console.log(wm.get(key)?.deref()); // [ 2 ]

Although keys are not strongly referenced in a `WeakMap`, values are, potentially leading to data retention issues and improper garbage collection. By utilizing `WeakRef`, this problem can be mitigated effectively. Weak references allow the garbage collector to collect an object even if it is only weakly referenced. This can prevent memory leaks in the injector debugger profiler.

Closes angular#55396
…eakRef` is defined

In some cases in G3 `WeakRef` is undefined because some tests are executed on older unsupported browsers.
@alan-agius4 alan-agius4 changed the title Injector profile changes fix(core): use WeakRef to prevent object retention in WeakMap Apr 26, 2024
@@ -21,7 +21,7 @@
"target": "es2022",
// Keep the below in sync with ng_module.bzl
"useDefineForClassFields": false,
"lib": ["es2020", "dom", "dom.iterable"],
"lib": ["es2020", "dom", "dom.iterable", "ES2021.WeakRef"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that you run into a #54051 situation :)

https://caniuse.com/mdn-javascript_builtins_array_at
https://caniuse.com/mdn-javascript_builtins_weakref

Of course it's totally in the realm of https://angular.dev/tools/cli/build#configuring-browser-compatibility; but a mention in the changelog or migration guide would be very appreciated for folks that like to include a polyfill for a while to not have that harsh of a cutoff (and not be surprised by error reporting).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's only used if the browser/environment supports it. You can read the rest of the PR, but basically this:

    // Only configure the injector profiler when running in the browser and WeakRef is defined.
    // In some G3 tests WeakRef is undefined as they user older (unsupported) browsers to test.
    if (typeof window !== 'undefined' && typeof WeakRef !== 'undefined') {

@josephperrott josephperrott removed their request for review May 1, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer 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: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

APP_INITIALIZER with HttpClient provoke memory leak in SSR projects
5 participants