fix(common): cleanup updateLatestValue if view is destroyed before promise resolves
#58041
Conversation
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
JeanMeche
reviewed
Oct 2, 2024
JeanMeche
reviewed
Oct 2, 2024
d25bcf2 to
b69033f
Compare
from() to prevent potential memory leaksupdateLatestValue if view is destroyed before promise resolves
b69033f to
c5e69fc
Compare
Contributor
|
@arturovt Can you rebase to resolve the conflicts here? |
c5e69fc to
c43b43e
Compare
c43b43e to
7ba238e
Compare
c42c2ff to
16d6986
Compare
…promise resolves According to the promise specification, promises are not cancellable by default. Once a promise is created, it will either resolve or reject, and it doesn't provide a built-in mechanism to cancel it. There may be situations where a promise is provided, and it either resolves after the pipe has been destroyed or never resolves at all. If the promise never resolves — potentially due to factors beyond our control, such as third-party libraries — this can lead to a memory leak. When we use `async.then(updateLatestValue)`, the engine captures a reference to the `updateLatestValue` function. This allows the promise to invoke that function when it resolves. In this case, the promise directly captures a reference to the `updateLatestValue` function. If the promise resolves later, it retains a reference to the original `updateLatestValue`, meaning that even if the context where `updateLatestValue` was defined has been destroyed, the function reference remains in memory. This can lead to memory leaks if `updateLatestValue` is no longer needed or if it holds onto resources that should be released. When we do `async.then(v => ...)` the promise captures a reference to the lambda function (the arrow function). When we assign `updateLatestValue = null` within the context of an `unsubscribe` function, we're changing the reference of `updateLatestValue` in the current scope to `null`. The lambda will no longer have access to it after the assignment, effectively preventing any further calls to the original function and allowing it to be garbage collected. If Chrome is built with additional flags and run with `--allow-natives-syntax --track-retaining-path`, we could use `%DebugTrackRetainingPath` to see the distance from the root for `updateLatestValue` if it's passed directly to async.then, e.g.: ```js %DebugTrackRetainingPath(updateLatestValue); // Distance from root 4: 0x123456789abc <JSPromise (sfi = 0x1fbb02e2d7f1)> ```
16d6986 to
362ee4f
Compare
pkozlowski-opensource
approved these changes
Apr 30, 2025
Contributor
Author
|
@thePunderWoman I would probably create another |
arturovt
added a commit
to arturovt/angular
that referenced
this pull request
Apr 30, 2025
…promise resolves Patch version of angular#58041.
Contributor
|
I think this PR would benefit from running a TGP to make sure there are no edge cases that we need to handle, adding the TGP label. |
Contributor
Contributor
|
Caretaker note: TGP is "green" after restarting some tests. |
Contributor
|
This PR was merged into the repository by commit 1bbf750. The changes were merged into the following branches: main, 20.0.x |
AndrewKushnir
pushed a commit
that referenced
this pull request
May 1, 2025
…promise resolves (#58041) According to the promise specification, promises are not cancellable by default. Once a promise is created, it will either resolve or reject, and it doesn't provide a built-in mechanism to cancel it. There may be situations where a promise is provided, and it either resolves after the pipe has been destroyed or never resolves at all. If the promise never resolves — potentially due to factors beyond our control, such as third-party libraries — this can lead to a memory leak. When we use `async.then(updateLatestValue)`, the engine captures a reference to the `updateLatestValue` function. This allows the promise to invoke that function when it resolves. In this case, the promise directly captures a reference to the `updateLatestValue` function. If the promise resolves later, it retains a reference to the original `updateLatestValue`, meaning that even if the context where `updateLatestValue` was defined has been destroyed, the function reference remains in memory. This can lead to memory leaks if `updateLatestValue` is no longer needed or if it holds onto resources that should be released. When we do `async.then(v => ...)` the promise captures a reference to the lambda function (the arrow function). When we assign `updateLatestValue = null` within the context of an `unsubscribe` function, we're changing the reference of `updateLatestValue` in the current scope to `null`. The lambda will no longer have access to it after the assignment, effectively preventing any further calls to the original function and allowing it to be garbage collected. If Chrome is built with additional flags and run with `--allow-natives-syntax --track-retaining-path`, we could use `%DebugTrackRetainingPath` to see the distance from the root for `updateLatestValue` if it's passed directly to async.then, e.g.: ```js %DebugTrackRetainingPath(updateLatestValue); // Distance from root 4: 0x123456789abc <JSPromise (sfi = 0x1fbb02e2d7f1)> ``` PR Close #58041
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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.
According to the promise specification, promises are not cancellable by default.
Once a promise is created, it will either resolve or reject, and it doesn't
provide a built-in mechanism to cancel it.
There may be situations where a promise is provided, and it either resolves after
the pipe has been destroyed or never resolves at all. If the promise never
resolves — potentially due to factors beyond our control, such as third-party
libraries — this can lead to a memory leak.
When we use
async.then(updateLatestValue), the engine captures a reference to theupdateLatestValuefunction. This allows the promise to invoke that function when itresolves. In this case, the promise directly captures a reference to the
updateLatestValuefunction. If the promise resolves later, it retains a referenceto the original
updateLatestValue, meaning that even if the context whereupdateLatestValuewas defined has been destroyed, the function reference remains in memory.This can lead to memory leaks if
updateLatestValueis no longer needed or if it holdsonto resources that should be released.
When we do
async.then(v => ...)the promise captures a reference to the lambdafunction (the arrow function).
When we assign
updateLatestValue = nullwithin the context of anunsubscribefunction,we're changing the reference of
updateLatestValuein the current scope tonull.The lambda will no longer have access to it after the assignment, effectively
preventing any further calls to the original function and allowing it to be garbage collected.
If Chrome is built with additional flags and run with
--allow-natives-syntax --track-retaining-path,we could use
%DebugTrackRetainingPathto see the distance from the root forupdateLatestValueif it's passed directly to async.then, e.g.: