undici fetch has memory leak

[cjh980402]

Bug Description

When I use node-fetch or axios, then there are no memory leak like under screenshot.
However, I found memory leak when I use undici.fetch.

Reproducible By

I made request about 50 times~200times then I found that memory leak easily.

Expected Behavior

there are no memory leak.

Logs & Screenshots

Environment

Ubuntu 20.04
Node v16.13.0

Additional context

[ronag]

Do you have a repro?

[cjh980402]

import { fetch } from 'undici';
import { setTimeout } from 'timers/promises';

const arr = [];
console.log(process.memoryUsage().heapUsed);

for (let i = 0; i < 200; i++) {
    if (i % 10 == 0) {
        console.log(process.memoryUsage().heapUsed);
    }
    for (let j = 0; j < 20; j++) {
        await (await fetch('https://www.youtube.com/')).text();
    }
    arr.push((await fetch('https://naver.com')).text());
}

await Promise.all(arr);
console.log(process.memoryUsage().heapUsed);

await setTimeout(10000);

console.log(process.memoryUsage().heapUsed);

await setTimeout(100000);

console.log(process.memoryUsage().heapUsed);

It has this result.

There are some memory leak so finally memory usage is increased.

[mcollina]

At first glance, your code behaves more or less how I expected. There should not be any memory leaks there.

(I should probably spend some time in documenting how this happen - however I'm a bit short on time right now).

[Ethan-Arrowood]

It would be valuable to try to reproduce this issue with just undici.request to isolate the issue to the fetch implementation.

Some areas within fetch it could happen is in the Body Mixin. We should ensure that once something like .text() is awaited that the stream is closed and destroyed.

[mcollina]

@Ethan-Arrowood My understanding is that there is no issue. All the requests are still referenced in the array and they can't be collected.

[ronag]

@cjh980402 can you try again with the latest release?

[cjh980402]

As a result of testing, it is much more stable than the previous version.
However, I will have to wait and see the situation by applying it to the actual application.

[kyrylkov]

I can confirm quite severe memory leak still present with Node v16.13.1 and undici v4.11.3

The issue is described in Node.js non-heap memory leak.

As soon as we replace undici with node-fetch, the issue disappears.

However do we know if the issue is with undici itself or with Web Streams / Buffer.blob?

Also switching back to node-fetch results in smaller GC pause times and smaller heap utilization. See chart below where just before 6pm node-fetch was deployed instead of unidici.

[ronag]

@kyrylkov: Can you show how you use fetch?

[kyrylkov]

@ronag There are multiple cases. Let me collect some.

[ronag]

Can you try with this branch? https://github.com/nodejs/undici/tree/finalization

[kyrylkov]

  const accessTokenRs = await fetch(URL, {
    method: 'POST',
    headers: {
      'Content-Type': 'application/json',
    },
    body: JSON.stringify(params),
  });
  const accessTokenRsJson = await accessTokenRs.json();

  await fetch(`${url}/lists/${listId}/members/${userHash}`, {
    method: 'DELETE',
    headers: {Authorization: BASIC_AUTH_KEY},
  });

  const res = await fetch(`${url}/lists/${listId}/members`, {
    method: 'GET',
    headers: {Authorization: BASIC_AUTH_KEY},
  });
  const resJson = await res.json();

  const res = await fetch(getUrl, {
    method: 'GET',
    headers:{
      'Authorization': `Bearer ${TOKEN}`,
    },
  });
  const text = await res.text();

[kyrylkov]

Can you try with this branch? https://github.com/nodejs/undici/tree/finalization

Trying....

[ronag]

await fetch(`${url}/lists/${listId}/members/${userHash}`, {
    method: 'DELETE',
    headers: {Authorization: BASIC_AUTH_KEY},
  });

You need to consume the response. See, https://github.com/nodejs/undici#garbage-collection.

[kyrylkov]

You need to consume the response. See, https://github.com/nodejs/undici#garbage-collection.

@ronag Alright. But this cannot cause the leak cause this specific code is called once per week at most.

[kyrylkov]

Can you try with this branch? https://github.com/nodejs/undici/tree/finalization

@ronag GC pauses immediately got at least 2x worse. Need more time to see if memory leak is still present.

[kyrylkov]

@ronag It's leaking again.

[mcollina]

The following does not leak memory:

import { fetch } from './index.js';
import { setTimeout } from 'timers/promises';

for (let i = 0; i < 200; i++) {
    if (i % 10 == 0) {
        console.log(process.memoryUsage().heapUsed / 1024 / 1024);
    }
    for (let j = 0; j < 20; j++) {
        await (await fetch('https://www.youtube.com/')).text();
    }
    await (await fetch('https://naver.com')).text();
}
console.log('all req sent');

console.log(process.memoryUsage().heapUsed / 1024 / 1024);

await setTimeout(10000);

console.log(process.memoryUsage().heapUsed / 1024 / 1024);

await setTimeout(100000);

console.log('end');

[kyrylkov]

@mcollina The leak we're observing is non-heap memory, so my understanding the test above wouldn't show it.

[mcollina]

Could you please include a test to reproduce?

What's exactly is the scale of those graphs?

What's the output you're seeing when Node is giving you the out of memory error?

[kyrylkov]

Could you please include a test to reproduce?

We don't have a test because it's a big app and we've been chasing non-heap memory leak for weeks. We seem to isolate the cause to undici fetch vs node-fetch.

What's exactly is the scale of those graphs?

Total memory on the chart in the Stack Overflow question is 768MB.

What's the output you're seeing when Node is giving you the out of memory error?

We would need to allow it to crash again to see exact out of memory error. The problem is that GC pauses increase 2x-3x as soon as we switch to undici fetch which negatively affects app performance in production. Thus running these tests till out of memory is both time consuming and bad for production application experience overall. We don't have a way to reproduce it outside of production environment.

[mcollina]

We would need to allow it to crash again to see exact out of memory error. The problem is that GC pauses increase 2x-3x as soon as we switch to undici fetch which negatively affects app performance in production. Thus running these tests till out of memory is both time consuming and bad for production application experience overall. We don't have a way to reproduce it outside of production environment.

The finalization branch linked above should fix/resolve it. This is actually pretty bad and possibly a reason to not be spec-compliant @ronag and drop the FinalizationRegistry.

[kyrylkov]

@mcollina What we can and will do next week is try to run for 24 hours with undici fetch, recording memory and GC profiles and then another 24 hours with node-fetch. That should answer lots of questions.

[mcollina]

We really need a way to reproduce the problem it we want to fix it.

[therapy]

heres a good way to reproduce the memory leak

index.js:

const { Worker } = require('worker_threads');
const os = require('os');

for (let i = 0; i < 50; i++) {
	new Worker('./worker.js');
}

setInterval(() => {
    console.log(os.freemem());
}, 1000)

worker.js

const { fetch } = require("undici");

const main = async () => {
    const res = await fetch("https://jsonplaceholder.typicode.com/posts/1");
    const json = await res.json();

    main();
};

main();

result with undici fetch:

8510468096
8388669440
8287883264
8248619008
8189575168
8142159872
8103874560
8013099008
7929524224
7844200448
7754084352
7699202048
7761747968
7981531136
8034414592
8069316608
8003137536
7938416640
7885611008
7810236416
7747260416
7687892992
7635963904
7576334336
7512018944
7481835520
7407083520
7363784704
7309643776
7222276096
7145009152
7120793600
7067721728
7029264384
6971269120
6915682304
6861324288
6814982144
6746136576
6654898176

exact same program with node-fetch:

8516784128
8501202944
8491171840
8480931840
8474705920
8485122048
8497430528
8497270784
8625246208
8608874496
8605544448
8605483008
8598585344
8599298048
8598294528
8597331968
8600895488
8595931136
8596119552
8594587648
8593002496
8598953984
8598147072

[mcollina]

I have tested this for about 10 minutes. Eventually it stabilizes but it degrades pretty sharply.
My understanding is this is somewhat related to the FinalizationGroup handling that we do with fetch() to handle some of the spec-required stuff.

@ronag I think we might want to remove it.

[ronag]

But we removed that?

[mcollina]

It seems it's still there:

undici/lib/fetch/request.js

Line 31 in 50fa51b

const requestFinalizer = new FinalizationRegistry(({ signal, abort }) => {

.

[szmarczak]

@mcollina This one has been added after the removal:

[mcollina]

I've tried removing that and it's not the cause or the problem.

Technically it's not a leak but there is some very odd memory behavior that we should fix.

[ronag]

I haven't really spent much time optimising allocations. It might also be related to web streams.

[mcollina]

I cannot reproduce it directly when using a single process. This is quite interesting:

const { fetch } = require(".");
const { isMainThread } = require('worker_threads');
const os = require('os');

const main = async () => {
    const res = await fetch("https://jsonplaceholder.typicode.com/posts/1");
    const json = await res.json();

    main();
};

main();
main();
main();
main();
main();

if (isMainThread) {
  setInterval(() => {
      console.log(os.freemem());
  }, 1000)
}

See

Very interestingly most of the allocations comes from Node core internals:

Going back to the multithreaded example, on my system the RSS stabilizes at 1.5 GB of RAM. My theory for this is that each worker uses about 30 MB of RAM (* 50 = 1.5 GB) because of the activity they need to do - this is consistent for the heap requirement of running a Node.js application.

See nodejs/node#34823 for more details on the topic of the memory requirements of worker threads.

Now, undici fetch() is not gentle on memory allocations and it should probably be optimize to reduce the memory consumption, but this can be closed.

[ronag]

Landing this in core will probably help some nodejs/node#41121

[mcollina]

Hopefully that'll happen soon.

[ronag]

Didn't make it into Node 17.3. Please try later with Node 17.4+ and Undici 4.12.1+

[ronag]

Cross that. It did make it into node 17.2. Please try with Node 17.2+ and Undici 4.12.1+.

[therapy]

heres a good way to reproduce the memory leak

index.js:

const { Worker } = require('worker_threads');
const os = require('os');

for (let i = 0; i < 50; i++) {
	new Worker('./worker.js');
}

setInterval(() => {
    console.log(os.freemem());
}, 1000)

worker.js

const { fetch } = require("undici");

const main = async () => {
    const res = await fetch("https://jsonplaceholder.typicode.com/posts/1");
    const json = await res.json();

    main();
};

main();

i have re-ran this:

https://pastebin.com/wqU6f14F

undici 4.12.1
node v17.3.0

looks like the memory usage is still very weird, but it seems to stabilize after 90 seconds while using 1.5gb of ram

I have tested this for about 10 minutes. Eventually it stabilizes but it degrades pretty sharply. My understanding is this is somewhat related to the FinalizationGroup handling that we do with fetch() to handle some of the spec-required stuff.

@ronag I think we might want to remove it.

I've tried removing that and it's not the cause or the problem.

Technically it's not a leak but there is some very odd memory behavior that we should fix.

Going back to the multithreaded example, on my system the RSS stabilizes at 1.5 GB of RAM. My theory for this is that each
worker uses about 30 MB of RAM (* 50 = 1.5 GB) because of the activity they need to do - this is consistent for the heap
requirement of running a Node.js application.