NextJS hot reload leaks memory

[yaronvhop]

Verify canary release

• I verified that the issue exists in Next.js canary release

Provide environment information

$ next info
'pnpm' is not recognized as an internal or external command,
operable program or batch file.

    Operating System:        
      Platform: win32        
      Arch: x64
      Version: Windows 10 Pro
    Binaries:
      Node: 16.14.0
      npm: 8.3.1
      Yarn: 1.22.11
      pnpm: N/A
    Relevant packages:       
      next: 12.1.1-canary.1  
      react: 17.0.2
      react-dom: 17.0.2    

What browser are you using? (if relevant)

98.0.4758.102

How are you deploying your application? (if relevant)

next dev

Describe the Bug

Hey,
It seems that nextjs leak all previous version of hot reload, even when they are old and not used:

as you can see from the picture _app.js _document.js are duplicated for each of their revision, even when not used at all, and even when closing the browser tab. It causes memory to go up until everything crashes on OOM error with some large projects.

Expected Behavior

Only one instance of each page/code should be kept in memory, otherwise this leak cause crash.

To Reproduce

https://github.com/smulikHakipod/nextjs-hot-reload-leak

Start NextJS in dev mode e.g npm run dev
Go to some page e.g index e.g http://127.0.0.1:3000/
Change a char in _app.js and save it, causing a hot reload.
Refresh the page (index) in the browser.
Repeat.
Watch memory grow and heap snapshot containing all duplicated pages content.
Repeat enough time, and it will crash or run out of system memory.

Thanks!!

[smulikHakipod]

I have read this entire thread #15855 without finding a reaoultion there

[balazsorban44]

It looks like the leak is related to when the devtool is open. Devtools keeps the source code of evaluated code alive as it can be displayed in the Sources panel. There seems to be a related Chrome bug:

https://bugs.chromium.org/p/chromium/issues/detail?id=1295659

Try avoiding using devtools for memory dumping. You could for example use heapdump instead which allows you to create snapshots without devtools. Also, make sure to use the latest Node.js version.

[smulikHakipod]

@balazsorban44 Thank you for the fast response.
I might not understand your comment, but I don't see how its related to Devtools.
The leak happens without dev-tools, or even when using Firefox as browser.
I used dev tools to showcase the issue, but its not seems related.

You can see from the pic above, NodeJS runs without debug mode at all in vs-code, and the browser is Firefox, no dev tools at sight, and still after few refreshes, process is at 2.2GB for this very small sample project.

[smulikHakipod]

I am using NodeJS 16.14.0 which the latest LTS as already stated

[lerot]

The issue reproduces on macos as well using node 16.13.1:

[sokra]

I think I traced that down to a memory leak in node.js introduced with node.js 16.0.0 with that commit:

nodejs/node@bf2f2b7#diff-c1d48dc599e8281b0be28ab449ea52917f6d4cc0578adb61ae99c631078b1a41R387

I tried with node.js 14.19.0 and it doesn't seem to have this leak.

The commit adds an object ({ importModuleDynamically: ... }) into a callbackMap for every executed function by the vm mode (which is internally used by the CommonJS Module loader).
Technically this callbackMap is a WeakMap, but it doesn't seem to cleanup the key.
The function that is passed as importModuleDynamically holds on the func variable, which keeps the source code of the function alive.

Here the latest version of the code in question: https://github.com/nodejs/node/blob/45b5ca810a16074e639157825c1aa2e90d60e9f6/lib/vm.js#L352-L383

But why is the key (cacheKey) not GC'ed. The cacheKey seem to be a plain js object created here: https://github.com/nodejs/node/blob/45b5ca810a16074e639157825c1aa2e90d60e9f6/src/node_contextify.cc#L1192-L1205

And there is also a C++ CompiledFnEntry which holds on the cacheKey object.
For the heapdumps it looks like this C++ object is a root object. I really unsure, but maybe one has to call entry->Detach() to avoid it being a root object. It's weakly attached to the Script object, and I think it should only be hold by that one...

If that is true the memory leak is maybe related to this commit nodejs/node@89e4b36
which makes CompiledFnEntry a BaseObject.

@addaleax @Trott could it be that CompiledFnEntry leaks memory? Do one have to call entry->Detach() to make it only be hold weakly by the script?

[sokra]

Here are bit more info from the heapdump:

The recursive trace looks also a bit suspicious to me, but not sure. Maybe CompiledFnEntry and cacheKey hold on each other recursively and that can't be handled because it's between C++ and JS object, I don't know...

[devsnek]

you might want to open an issue on the node repo.