-
Notifications
You must be signed in to change notification settings - Fork 538
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
Use FinalizationRegistry to dump the socket if the Response is GC'd #2856
Comments
I thought this was implemented in this way but I could not substantiate it with code. |
To really get it spec compliant:ish we would need force gc at some interval? Or we could run out of socket handles. |
Without consuming the body I couldn't get node to gc the Response returned from fetch |
And that happens due to WebStreams or also with normal Streams? |
I think you got it backwards. We need to use a Line 165 in 4090043
Request and Response of a given fetch. I think some WeakRef could allow it to be collected, and therefore GC'd.
|
no luck with that either, there are quite a few things that also have strong references to them (requestObject, controller, etc.). It'd be pretty messy dealing with all of them. diffdiff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js
index e7115b2e..84a7a2cb 100644
--- a/lib/web/fetch/index.js
+++ b/lib/web/fetch/index.js
@@ -64,12 +64,18 @@ const { dataURLProcessor, serializeAMimeType, minimizeSupportedMimeType } = requ
const { getGlobalDispatcher } = require('../../global')
const { webidl } = require('./webidl')
const { STATUS_CODES } = require('node:http')
+const { FinalizationRegistry } = require('./dispatcher-weakref')()
+
const GET_OR_HEAD = ['GET', 'HEAD']
const defaultUserAgent = typeof __UNDICI_IS_NODE__ !== 'undefined' || typeof esbuildDetection !== 'undefined'
? 'node'
: 'undici'
+const registry = new FinalizationRegistry((response) => {
+ console.log(response)
+})
+
/** @type {import('buffer').resolveObjectURL} */
let resolveObjectURL
@@ -188,7 +194,10 @@ function fetch (input, init = undefined) {
// 4. Abort the fetch() call with p, request, responseObject,
// and requestObject’s signal’s abort reason.
- abortFetch(p, request, responseObject, requestObject.signal.reason)
+ const _responseObject = responseObject.deref()
+ if (_responseObject) {
+ abortFetch(p, request, responseObject, requestObject.signal.reason)
+ }
}
)
@@ -215,8 +224,12 @@ function fetch (input, init = undefined) {
// 2. Abort the fetch() call with p, request, responseObject, and
// deserializedError.
+ const _responseObject = responseObject.deref()
+
+ if (_responseObject) {
+ abortFetch(p, request, _responseObject, controller.serializedAbortReason)
+ }
- abortFetch(p, request, responseObject, controller.serializedAbortReason)
return
}
@@ -229,7 +242,8 @@ function fetch (input, init = undefined) {
// 4. Set responseObject to the result of creating a Response object,
// given response, "immutable", and relevantRealm.
- responseObject = fromInnerResponse(response, 'immutable', relevantRealm)
+ responseObject = new WeakRef(fromInnerResponse(response, 'immutable', relevantRealm))
+ registry.register(responseObject, response)
// 5. Resolve p with responseObject.
p.resolve(responseObject) |
I'll take a look as well. How do you test it? |
const { fetch } = require('./index.js')
async function run () {
await fetch('https://example.com')
}
run()
// otherwise process will end
setTimeout(() => {}, 120000) |
If a user can't consume the body anymore, we can just dump it.
We can detect a response is not accessible by the use of a
FinalizationRegistry
.@KhafraDev I think this would bring us a step closer to the spec.
The text was updated successfully, but these errors were encountered: