Memory leak caused by ChunkExtractor

[floric]

🐛 Bug Report

Server side rendering in combination with specifying the stats file path for the ChunkExtractor seems to cause a memory leak. It might be in connection with streaming the response, but I'm not sure about this yet. Lucky wise I found a better solution and think we should adapt the documentation to preload the file only once anyway. Whats the point of loading it for every request again? Do you agree and maybe know the issue of the memory leak?

To Reproduce

Example for Fastify:

const chunkExtractor = new ChunkExtractor({ statsFile: path });
  const jsx = await createReactTree(chunkExtractor);
  const resStream = new PassThrough();
  reply
    .headers({})
    .status(200)
    .send(resStream);
  resStream.write(buildHeader(chunkExtractor));
  const stream = renderToNodeStream(jsx);
  await new Promise((resolve, reject) => {
    stream.pipe(resStream, { end: false });
    stream.on("error", err => {
      resStream.end();
      reject(err);
    });
    stream.on("end", () => {
      resStream.write(buildFooter(client));
      resStream.end();
      resolve();
    });
  });

Expected behavior

ChunkExtractor loads stats and releases all allocated memory after response is completed.

Used solution

I was able to solve it in my case by simply loading the file once during startup. If the file otherwise gets loaded for every request, then the examples and documentation should also be changed, as it will decrease performance and doesn't seem to be necessary:

const statsFile = resolve(__dirname, "loadable-stats.json");
const stats = JSON.parse(readFileSync(statsFile).toString("utf8"));

const doAnswer = async (req, res) => {
   const chunkExtractor = new ChunkExtractor({ stats });
   // more code
}

Link to repl or repo

Unfortunately, this is private project. But as I already found a better solution, I just like to inform you in case you might be aware of the underlying issue.

System:

• OS: Linux 5.5 Fedora 31 (Workstation Edition) 31 (Workstation Edition)
• CPU: (8) x64 Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz
• Memory: 23.52 GB / 31.08 GB
• Container: Yes
• Shell: 5.0.11 - /bin/bash

Binaries:

• Node: 10.16.0 - ~/.nvm/versions/node/v10.16.0/bin/node
• Yarn: 1.22.4 - /usr/bin/yarn
• npm: 6.9.0 - ~/.nvm/versions/node/v10.16.0/bin/npm

npmPackages:

• @loadable/babel-plugin: ^5.11.0 => 5.12.0
• @loadable/component: ^5.11.0 => 5.12.0
• @loadable/server: ^5.11.0 => 5.12.0
• @loadable/webpack-plugin: ^5.7.1 => 5.12.0

[open-collective-bot]

Hey @floric 👋,
Thank you for opening an issue. We'll get back to you as soon as we can.
Please, consider supporting us on Open Collective. We give a special attention to issues opened by backers.
If you use Loadable at work, you can also ask your company to sponsor us ❤️.

[theKashey]

Yes, it is a memory leak, hopefully only in dev - here

loadable-components/packages/server/src/util.js

Lines 3 to 6 in 4e99a97

	export const smartRequire = modulePath => {
	if (process.env.NODE_ENV !== 'production') {
	clearModuleCache(modulePath)
	}

The module cache got cleared, so file could be required again, but it does not clean reference from the parent module, so there is more and more copies stored in the "parent module" (which is util.js)

Open questions:

• which way - stat or statFile - is better for dev? For production there is no difference
• why stats has to be updated so "forceably"? With HMR you don't have to remove something from the cache
• should one keep util module cache clean? Ie wipe data from the require[module].children

[floric]

"Great" :)

I'm not sure about the correct way yet. I don't use HMD but reloading the server with Webpack in watch mode together with Nodemon and stat seems to work fine.

Somehow I'm confused that there is an ENV check. I thought it should already be executed with NODE_ENV set to production. I will check this on my side.

[theKashey]

Refs:

• Wiping cache element is keeping it in the parent .children list theKashey/wipeNodeCache#4
• reference to a module could be kept in the children theKashey/rewiremock#116