From 1bf522455709bda997c8ff79b401723181bf2731 Mon Sep 17 00:00:00 2001 From: Suneil Nyamathi Date: Thu, 8 Aug 2024 15:46:29 -0700 Subject: [PATCH 1/3] fix: memory leak Holding a reference to the stream in the finalization registry causes a memory leak, even when consuming the body. --- lib/web/fetch/response.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index 8c00835698e..684cdad60ee 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -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) } }) @@ -526,7 +527,7 @@ function fromInnerResponse (innerResponse, guard) { setHeadersGuard(response[kHeaders], guard) if (hasFinalizationRegistry && innerResponse.body?.stream) { - registry.register(response, innerResponse.body.stream) + registry.register(response, new WeakRef(innerResponse.body.stream)) } return response From b987f9389b203f8ebb338c6c0670fcfbc76ee2e6 Mon Sep 17 00:00:00 2001 From: Suneil Nyamathi Date: Thu, 8 Aug 2024 21:27:20 -0700 Subject: [PATCH 2/3] docs: add comment explaining the strong reference --- lib/web/fetch/response.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index 684cdad60ee..30ede0ada3a 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -527,6 +527,11 @@ function fromInnerResponse (innerResponse, guard) { setHeadersGuard(response[kHeaders], guard) if (hasFinalizationRegistry && 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). Rewording from + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry registry.register(response, new WeakRef(innerResponse.body.stream)) } From 435ad9e147dfef688cd4e8688c67c73bbc892f65 Mon Sep 17 00:00:00 2001 From: Suneil Nyamathi Date: Thu, 8 Aug 2024 21:27:52 -0700 Subject: [PATCH 3/3] typo --- lib/web/fetch/response.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index 30ede0ada3a..603410a4a63 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -530,7 +530,7 @@ function fromInnerResponse (innerResponse, guard) { // 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). Rewording from + // 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)) }