Potential memory leak

[sharq88]

Describe the bug
We have been hunting a memory leak for quite a while in our Node.JS app, and traced it back to chartjs-node-canvas source code. I have created a simplified / standalone demo for easy reproduction (attached).

If you run npm i && node index open browser and run http://localhost:3000/test a few times, then open http://localhost:3000/reveal

Since the chart.js module is deleted from require cache: https://github.com/SeanSobey/ChartjsNodeCanvas/blob/master/src/index.ts#L274
during initialisation but the module.children is not handled it adds a new instance of chart.js into the global scope (require cache) every time a new instance is created. (I'm aware that its recommended to reuse the chartjs instance, however its not an option in our project. We have to create a new instance every time.)

In which case heap grows until it reaches memory limit, then GC goes crazy to free up space eventually falls over with heap out-of-memory.

I read in readme:

Uses (similar to) fresh-require for each instance of ChartJSNodeCanvas, so you can mutate the ChartJS global variable separately within each instance."

and was thinking on

• creating a fork, but then other people out there might experience the same issue
• submitting a PR but I wasn't exactly sure my changes would not cause any breaks (due to my limited understanding on internal workings)

so I thought its props wiser to bring it to your attention and discuss first.

Btw thanks for your hard work on this project! Its very useful!!!

Versions

• NodeJS version: 18.16.0
• Chart.JS version: 3.9.1
• ChartJSNodeCanvas version: 4.1.6

Additional context

require.cache.leak.zip

[sharq88]

Bad news.. from official support perspective: nodejs/node#8443

[sharq88]

I have rewritten part of chartjs-node-canvas. It does need some more work on the typescript front. (dist/index.js contains a clean and working version which does not leak memory. This could be ported back to the /src folder) freshRequire is now removed I can see it caused quite a lot of issues for many people. Using ESM there is no need for freshRequire or similar workarounds because modules are not cached. (nodejs/help#2806) I've moved background colour plugin into index. If no other default modules are added its simpler to keep/maintain it in one file IMO, but I don't really mind either way. Attempted to refactor all references of CJS to EJS and checked async wrappers too. Since this pull contains breaking changes, If this ever gets merged I'd recommend to bump and release as major version change.

UPDATE: Unfortunately the above did not work. As I read ESM does not have require cache, but it does not mean its not caching modules unfortunately.. memory use was steady because it reused the module which is not ideal here.

[sharq88]

Fixed... after investing quite a lot of time into it: sharq88#1 I’ve built a fake ChartJSNodeCanvas class with identical interface and proxied all function calls to a child process factory. Whenever the constructor of ChartJSNodeCanvas is called it creates a new child process, which has nothing else but the chartjs library included. Whenever a function is called in the proxy, the handle executes the function in the child process and returns the response via IPC channels. Since the constructor is sync but most functions are async I’ve applied another trick to check readiness using promises. It runs slightly slower due to the addition IPC conversions, however memory usage is stabilised, which for some worth the compromise. :) to use in your project replace relevant package.json line to:
"chartjs-node-canvas" : "https://github.com/sharq88/ChartjsNodeCanvas.git",

(As a feedback - please throw a thumbs up if it helped you - thanks)