Skip to content

fix: increased memory in finalization first appearing in v6.16.0 #3445

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

Merged
merged 3 commits into from
Aug 9, 2024
Merged

fix: increased memory in finalization first appearing in v6.16.0 #3445

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1bf5224
fix: memory leak
snyamathi Aug 8, 2024
b987f93
docs: add comment explaining the strong reference
snyamathi Aug 9, 2024
435ad9e
typo
snyamathi Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/web/fetch/response.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ const hasFinalizationRegistry = globalThis.FinalizationRegistry && process.versi
let registry

if (hasFinalizationRegistry) {
registry = new FinalizationRegistry((stream) => {
if (!stream.locked && !isDisturbed(stream) && !isErrored(stream)) {
registry = new FinalizationRegistry((weakRef) => {
const stream = weakRef.deref()
if (stream && !stream.locked && !isDisturbed(stream) && !isErrored(stream)) {
stream.cancel('Response object has been garbage collected').catch(noop)
}
})
Expand Down Expand Up @@ -526,7 +527,12 @@ function fromInnerResponse (innerResponse, guard) {
setHeadersGuard(response[kHeaders], guard)

if (hasFinalizationRegistry && innerResponse.body?.stream) {
registry.register(response, innerResponse.body.stream)
// If the target (response) is reclaimed, the cleanup callback may be called at some point with
// the held value provided for it (innerResponse.body.stream). The held value can be any value:
// a primitive or an object, even undefined. If the held value is an object, the registry keeps
// a strong reference to it (so it can pass it to the cleanup callback later). Reworded from
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry
registry.register(response, new WeakRef(innerResponse.body.stream))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

registry.register(target, "some value");

If target is reclaimed, your cleanup callback may be called at some point with the held value you provided for it ("some value" in the above). The held value can be any value you like: a primitive or an object, even undefined. If the held value is an object, the registry keeps a strong reference to it (so it can pass it to your cleanup callback later).

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry

Copy link
Member

Choose a reason for hiding this comment

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

if no one merges this before you can, adding this as a comment in the code itself would be appreciated

}

return response
Expand Down