unidict fetch leaked if using Request object as argument

[stalkerg]

Bug Description

unidict leaked if using Request, for example:

export function create_fetch({ event, options, state, get_cookie_header }) {
	return async (info, init) => {
		return await fetch(new Request(info, init));
	};
}

I found it in SvelteKit inside this issue sveltejs/kit#8239

Reproducible By

See SvelteKit issue.

Environment

Ubuntu and Gentoo with the latest node 18

[stalkerg]

Seems like we trying to copy AbortController signal from the income Request here:
https://github.com/nodejs/undici/blob/main/lib/fetch/request.js#L106

and after locking into the arrow function, such signal from the previous Request:
https://github.com/nodejs/undici/blob/main/lib/fetch/request.js#L351

seems like we never run finalization for the original signal. Not sure here, but a bug exists... maybe we should do clone instead copy only a few fields?

[ronag]

Isn't this just a case of the finalisation registry not yet running the finaliser?

[stalkerg]

hmm seems like it's never running, and at the same time this:

export function create_fetch({ event, options, state, get_cookie_header }) {
	return async (info, init) => {
		return await fetch(info, init);
	};
}

working prefect.

[stalkerg]

Seems like this https://github.com/nodejs/undici/blob/main/lib/fetch/request.js#L337 is needed only if init.signal exist, for the case when we just copy the ac signal from the original Request we shouldn't follow it.

[stalkerg]

As I understand, we shouldn't recreate the Request inside fetch if we have already received the Request.
Even if we made Request based on a Request, it should be copied without the following:
https://developer.mozilla.org/en-US/docs/Web/API/Request/Request#parameters

Maybe I do not fully understand the concept behind this, but it seems like we are doing something wrong now.

[stalkerg]

Sorry, for too many words here. I checked the specification https://fetch.spec.whatwg.org/#fetch-method and it seems like, currently, everything is implemented right. I still have questions about such specifications but this question is outside this issue.
Also, seems like SveltKit fetch like wrapper is not fully correct just because it's not a fetch anymore, but it's also a different topic.

@ronag big thanks for a fast fix.

[stalkerg]

@ronag can you reopen? The latest changes make memory leak again.

[ronag]

@stalkerg We need a minimal repro to properly solve this.

[stalkerg]

@ronag can you try repro from #46435 ? But seems like this time, the reason for the memory leaks is not in undici but I am still trying to find. It's surprisingly coincident with SvelteKit update to the latest undici version which was reverting previous changes. :)
sveltejs/kit#9512

[stalkerg]

Okey, it's undici again because the new version is now merged into shims.js of adapter-node in SvelteKit. https://github.com/sveltejs/kit/blob/master/packages/adapter-node/src/shims.js

Yes, I can confirm now the new undici memory leaked again.

[jamesdiacono]

The reproduction in #46435 causes a memory leak in Node.js v19.8.1.

[stalkerg]

@jamesdiacono thanks, I had a theory that it was fixed in v19.3 but seems like no, and we have some GC issue that can't collect self-linked things properly.
Interesting can we make repro without undici code?

[jamesdiacono]

The problem was fixed, but has now regressed. I have only experienced the leak in relation to fetch.