Skip to content
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

Memory leak if add external fetch inside load function #8239

Closed
stalkerg opened this issue Dec 21, 2022 · 10 comments
Closed

Memory leak if add external fetch inside load function #8239

stalkerg opened this issue Dec 21, 2022 · 10 comments
Labels
blocked by upstream bug Something isn't working

Comments

@stalkerg
Copy link
Contributor

Describe the bug

Very fast memory leak, basically we hold all response strings and all fetch data from remote servers. It seems like because we have circular links for abort callback inside node-fetch(it uses WeakRef and etc, but it's not working).
изображение
изображение

Reproduction

Create a new test project from the starter and add into +layout.server.js the load function:

import { error } from '@sveltejs/kit';

/** @type {import('./$types').LayoutServerLoad} */
export async function load({ fetch }) {
  
  let res;
  try {
    res = await fetch(
      `http:https://localhost:8989/api/v3/profile?lang=en`,
      {
        credentials: 'include',
      },
    );
  } catch (err) {
    console.error(err);
    return;
  }

  if (res.status !== 200) {
    console.error('Could not load profile', res.status, res.url);
    throw error(500, 'Could not load profile');
  }

  const { env, user } = await res.json();

  return {
    sessionUser: user,
    env,
  };
}

it's can be any other external (outside sveltekit) request. And run wrk or ab over any page.
You can build it for the node adapter and run it or it works even in dev.
I am use next command to debug:
ORIGIN=http:https://localhost:3000 /usr/bin/node --es-module-specifier-resolution=node --max_old_space_size=200 --inspect --expose-gc build/

Logs

No response

System Info

System:
    OS: Linux 6.1 Gentoo Linux
    CPU: (24) x64 AMD Ryzen 9 5900X 12-Core Processor
    Memory: 10.09 GB / 31.26 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 18.12.1 - /usr/bin/node
    npm: 8.19.2 - /usr/bin/npm
  npmPackages:
    @sveltejs/adapter-auto: ^1.0.0 => 1.0.0 
    @sveltejs/adapter-node: ^1.0.0 => 1.0.0 
    @sveltejs/kit: ^1.0.0 => 1.0.1 
    svelte: ^3.54.0 => 3.55.0 
    vite: ^4.0.0 => 4.0.2

Severity

blocking all usage of SvelteKit

Additional Information

I found this after deploying my project to production... now I am in a really bad situation because nodejs process is restarted each 30mins.

@stalkerg
Copy link
Contributor Author

If I am using fetch from the node directly instead load function argument - this issue is gone.

@stalkerg
Copy link
Contributor Author

hmm because of this https://github.com/sveltejs/kit/blob/master/packages/kit/src/exports/node/polyfills.js#L1 even if nodejs support fetch we are using undici implementation.

@dummdidumm
Copy link
Member

hmm because of this https://github.com/sveltejs/kit/blob/master/packages/kit/src/exports/node/polyfills.js#L1 even if nodejs support fetch we are using undici implementation.

The native fetch implementation of node is undici, so there's no difference (except the undici version which is newer through the polyfill)

@stalkerg
Copy link
Contributor Author

okey, I found the root of the leak it's how we tricky use handleFetch here
https://github.com/sveltejs/kit/blob/master/packages/kit/src/runtime/server/fetch.js#L25

if I return our internal fetch as is, it's working fine, but if by:
(({ request, fetch }) => fetch(request)) it's leaked.

@stalkerg
Copy link
Contributor Author

stalkerg commented Dec 21, 2022

sorry, it's a little bit different: if I use return await fetch(info, init) it has no leak but if I start using return await fetch(original_request) it's leaked

I am hacking now create_fetch function.

UPDATE:
yes, even this stub is leak:

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

but this has not leaked:

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

it means Request itself leaked or how fetch working with Request.

@stalkerg
Copy link
Contributor Author

okey, hot fix is here nodejs/undici#1824 seems like it's a serious issue, and we must wait for a new release.

@dummdidumm dummdidumm added bug Something isn't working blocked by upstream labels Dec 21, 2022
@machak
Copy link

machak commented Jan 11, 2023

@stalkerg
Copy link
Contributor Author

Cool! Now we should update deps for kit.

@dummdidumm
Copy link
Member

Happened in #8459, release soon

@stalkerg
Copy link
Contributor Author

stalkerg commented Mar 25, 2023

They break it again nodejs/undici#2000

Can we revert undici update?
@dummdidumm

javiersanp added a commit to OSM-es/visor-catastro that referenced this issue May 15, 2023
sveltejs/kit#8239
sveltejs/kit#9427
Hay que vigilar si se resuelve el problema de undici
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked by upstream bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants