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

test: add test for memory leak #3450

Merged
merged 2 commits into from
Aug 14, 2024
Merged

test: add test for memory leak #3450

merged 2 commits into from
Aug 14, 2024

Conversation

snyamathi
Copy link
Contributor

@snyamathi snyamathi commented Aug 12, 2024

@mcollina I was able to finally get a minimal reproduction without using next or react

https://github.com/snyamathi/undici-mem-leak

chart2

Looking at the chart, it does indeed appear to be a memory leak but it seems to be specific to Node's AsyncLocalStorage which is something that I dug out of the React source code here

This may actually have a root issue in Node itself which the team may want to be aware of - it seems like the reference prevents garbage collection so this same bug could bite others using AsyncLocalStorage in the same way.


The test is a little round-a-bout in its testing, but it tries to assert that the body is garbage collected by checking that the weak ref becomes undefined.

This test fails before the change that I put in, and only succeeds after.


I have the reproduction setup to test multiple versions of undici and show the problem does not exist in 6.15.0, does exist in 6.16.0 and is then fixed in 6.19.7 - check out the repo link above, but it looks like this:

import { setImmediate } from "node:timers/promises";
import { AsyncLocalStorage } from "node:async_hooks";

const { UNDICI_VERSION = "6.19.7" } = process.env;
const { Response } = await import(`undici-${UNDICI_VERSION}`);
const asyncLocalStorage = new AsyncLocalStorage();

const res = new Response("hello world");
const format = (bytes) => Math.round(bytes / 1024 ** 2) + "MB";

for (let i = 0; ; ++i) {
  if (i % 100 === 0) {
    console.log(i, format(process.memoryUsage().heapUsed));
  }

  asyncLocalStorage.run(new Map(), async () => {
    const clone = res.clone();
    asyncLocalStorage.getStore().set("key", clone);
    await clone.text(); // consume body
  });

  await setImmediate(); // allow gc
}

related to #3445

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. This confirms my suspects that the Response is actually not getting collected at the right moment, because the store is being kept alive. I'm trying to understand why that happens, but the fix is solid.

What puzzles me a bit is that over time, the memory consumption seems stable. I've not been able to generate a crash from this, so eventually things will get collected, but this particular loop of objects is pretty hard for the GC to track down.

@ronag ronag merged commit 27f46c1 into nodejs:main Aug 14, 2024
32 checks passed
mcollina pushed a commit that referenced this pull request Aug 17, 2024
* test: add test for memory leak

* lint
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants