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
base: main
Are you sure you want to change the base?
Conversation
Caretaker note: this requires cl/627330384 to be landed in G3 |
8ec9b40
to
82e7b20
Compare
There was a problem hiding this 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
resolverToTokenToDependencies = new WeakMap< | ||
Injector|LView, WeakMap<Type<unknown>|InjectionToken<unknown>, WeakRef<InjectedService[]>>>(); | ||
resolverToProviders = new WeakMap<Injector|TNode, WeakRef<ProviderRecord[]>>(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
82e7b20
to
3aa0b95
Compare
WeakRef
to prevent object retention in WeakMap
@@ -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"], |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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') {
Although keys are not strongly referenced in a
WeakMap
, values are, potentially leading to data retention issues and improper garbage collection. By utilizingWeakRef
, 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