node icon indicating copy to clipboard operation
node copied to clipboard

Increased memory usage in nodejs version 20.16.0 due to fetch()

Open BridgeAR opened this issue 1 year ago • 21 comments

Discussed in https://github.com/orgs/nodejs/discussions/54248

Originally posted by SteffenHo August 7, 2024 We saw a massive increase in memory in our next.js application after updating to version 20.16.0. We build our application with docker and the 20-alpine image. Other users were also able to confirm this problem: https://www.reddit.com/r/node/comments/1ejzn64/sudden_inexplicable_memory_leak_on_new_builds/

image

We then switched to version 20.15.1 and the memory returned to normal.

Maybe someone else could confirm this problem too?

@ronag could you have a look into this?

BridgeAR avatar Aug 08 '24 21:08 BridgeAR

FWIW @snyamathi believes that this is caused by https://github.com/nodejs/undici/commit/63b7794af7c282d543a56d13521f9d5e92fa61ae

CC @nodejs/undici

avivkeller avatar Aug 08 '24 21:08 avivkeller

Possible fix in https://github.com/nodejs/undici/pull/3445

trivikr avatar Aug 08 '24 22:08 trivikr

@mcollina @Uzlopak seems related to fetch and finalization.

ronag avatar Aug 09 '24 03:08 ronag

First, a memory leak happens when the memory is never released. In the diagram above, the gc is at work in claiming/releasing all the memory. Therefore, this bug is not a memory leak, but a less panicking "increased memory usage" bug.

Second, I would really love to see a reproduction that is not using Next.js, because it monkey-patches fetch()... which might interact badly with the "fire and forget" logic of undici (we need to support fetches that do not consume the request).

mcollina avatar Aug 09 '24 06:08 mcollina

I've done a bit of research on this problem, and I came to these conclusions:

  • Next.js monkey patches global fetch() in https://github.com/vercel/next.js/blob/9a1cd356dbafbfcf23d1b9ec05f772f766d05580/packages/next/src/server/lib/patch-fetch.ts#L786-L796.
  • Using vanilla undici in the provided example does not show the problem, this is also highlighted in https://github.com/vercel/next.js/discussions/68636#discussioncomment-10270652.
  • https://github.com/nodejs/undici/pull/3445 improves memory consumption by roughly 10% in my tests without visible issues. I couldn't replicate whether it fixes the problem, but I'll ship a release with this anyway, as it's a good improvement. I don't expect it to fix the problem.

Something in the latest undici releases increased memory consumption for the Next.js cache. This does not affect vanilla fetch() users.

We need a reproduction without Next.js or React.cache() that clearly shows the problem. I couldn't isolate it myself, and I've exhausted the time I could dedicate so far.

I've been trying to follow what their patching does, but I were not able to identify the problem.

mcollina avatar Aug 09 '24 08:08 mcollina

https://github.com/nodejs/node/pull/54286 contains the patched version of undici.

mcollina avatar Aug 09 '24 16:08 mcollina

https://github.com/nodejs/node/pull/54286 contains the patched version of undici.

That PR has landed, so I've closed this issue. Feel free to reopen if you disagree.

avivkeller avatar Aug 10 '24 02:08 avivkeller

I am encountering this with an Express app (not Next.js) and it appears to be an actual leak in my case. The heap grows until the process crashes with FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory. It crashes eventually even with --max-old-space-size=1000, while on Node 20.15.1, it remains stable permanently with --max-old-space-size=50.

I'll try distilling to a minimal repro case, it sounds like that could still be useful? Or are we likely all set with that PR?

mistval avatar Aug 14 '24 13:08 mistval

Open a new issue, this case is very specific to Next.js, so it'll likely be something else.

mcollina avatar Aug 14 '24 13:08 mcollina

We encountered this bug and confirmed that it still exists in 20.17. If you run into this issue, please use 20.15.1 temporarily. Upgrading to latest nodejs v22 might also help

kouhin avatar Aug 27 '24 02:08 kouhin

@kouhin the fix landed in undici v6.19.6 and Node 20.17 still uses undici v6.19.2

https://github.com/nodejs/undici/compare/v6.19.5...v6.19.6

https://github.com/nodejs/node/blob/v20.17.0/src/undici_version.h#L5


edit: I've opened a PR to backport the fix to Node v20 https://github.com/nodejs/node/pull/54583

snyamathi avatar Aug 27 '24 02:08 snyamathi

The fix shipped in undici v6.19.7, which was shipped in Node v22.7.0. This will be part of the next v20 release if it happens after 2 weeks.

@nodejs/releasers

mcollina avatar Aug 27 '24 05:08 mcollina

reopening to track until this ships in v20.

mcollina avatar Aug 27 '24 05:08 mcollina

@mcollina we are on v20, is there a suggested workaround rather than going to v22 or waiting for the next release or do we downgrade to 20.15.1 and use --max-old-space-size?

mrtimp avatar Aug 28 '24 03:08 mrtimp

After reverting to 20.15.1 we now have a more stable memory utilisation (AWS ECS Fargate):

screenshot_ms_832

mrtimp avatar Aug 29 '24 01:08 mrtimp

@mcollina we are on v20, is there a suggested workaround rather than going to v22 or waiting for the next release or do we downgrade to 20.15.1 and use --max-old-space-size?

You can always download the latest undici from npm and use that?

avivkeller avatar Aug 29 '24 17:08 avivkeller

@RedYetiDev we tried that however it didn’t work for us

mrtimp avatar Aug 29 '24 18:08 mrtimp

You may be experiencing a different issue, if so, please open in issue in nodejs/help or nodejs/undici

avivkeller avatar Aug 29 '24 18:08 avivkeller

This is getting off topic for this issue, but the global fetch (https://nodejs.org/dist/latest-v20.x/docs/api/globals.html#fetch) which comes with node itself is different than adding a dependency on the npm package undici

In theory, you could patch the global fetch with the npm package, though in practice you're probably better off just sticking to a version before this issue was present (eg 20.15.1) or after the patch is backported (TBD).

The reason you don't want to patch fetch is because (assuming you're using it) both Next.JS AND React separately patch the global fetch function which may cause issues if done out of order. You could get around this by preloading the patch at startup, but why complicate things. Just downgrade or wait 😄

snyamathi avatar Aug 29 '24 19:08 snyamathi

Adding up, had to downgrade to 20.15.1 to fix the leak on my project with Next.js (v14.2.5 - this didn't change). Will probably stick to node 20.15.1 as 20.17 LTS causes the leak.

@RedYetiDev downloading undici would do the trick at the cost of having yet another dependency (and we should remind to remove it sooner or later when we'll upgrade node).

edoardo-liotta avatar Aug 30 '24 08:08 edoardo-liotta

Any ETA on this get back port to v20?

Fonger avatar Sep 24 '24 06:09 Fonger

I have the same problem with Next.js and Node v20.16 and v20.17

image (1)

jnm733 avatar Oct 03 '24 14:10 jnm733

The fix shipped in Node.js v20.18.0

mcollina avatar Oct 04 '24 07:10 mcollina

Does anyone know if this also affected node v20.11.1? We're seeing a potential memory leak in next v14.2.7 with the pages router, but it's not as obvious with next v13.5.6, so I'm struggling to find the real issue.

YutaMoriJP avatar Nov 19 '24 06:11 YutaMoriJP