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

Zlib reserving twice the needed memory for inflate and inflateSync, kept in RSS until process exits. #44750

Closed
nitram-work opened this issue Sep 22, 2022 · 5 comments

Comments

@nitram-work
Copy link

nitram-work commented Sep 22, 2022

Version

v18.9.0 and v16.16.0

Platform

Linux 5.15.0-48-generic #54~20.04.1-Ubuntu SMP Thu Sep 1 16:17:26 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

zlib

What steps will reproduce the bug?

In the following zip file it's a copy of this JS code and the binary file needed to consistently cause the issue.
IMPORTANT: Needs to be called with node --expose-gc

inflate-leak.zip

const fs = require("fs");
const zlib = require("zlib");

const inMB = (b) => (b / 1024 / 1024).toFixed(3) + " MB";

const logMem = async (when) => {
  console.log('-------' + when.padEnd(70, '-'));
  global.gc();
  await new Promise((resolve) => setTimeout(resolve, 1000));
  const mu = process.memoryUsage();
  for (let k in mu) {
    console.log(`${k.padStart(20)} : ${inMB(mu[k])}`);
  }
  await new Promise((resolve) => setTimeout(resolve, 1000));
};



const main = async () => {
  await logMem("initial");
  let deflated = fs.readFileSync("inflate-leak.bin");
  await logMem(`deflated ${inMB(deflated.length)}`);

  let inflated = zlib.inflateSync(deflated);
  //let inflated = await new Promise(resolve => zlib.inflate(deflated, (err, buf) => resolve(buf)));
  await logMem(`inflated size ${inMB(inflated.length)}`);
  inflated = null;
  await logMem("inflated=null");
  deflated = null;
  await logMem("deflated=null");

  for(let i = 0; i<100; i++)
    global.gc();
  
  await new Promise((resolve) => setTimeout(resolve, 10_000));
  await logMem("after 10 seconds");
};

main().then();

How often does it reproduce? Is there a required condition?

Every time for the attached binary file.

What is the expected behavior?

Inflate reserving only the needed size (~1.6GB) instead of double (~3.2GB).
RSS lowered reflecting array buffers/external lowered size after garbage collection.

What do you see instead?

After inflate or inflateSync RSS stays with an extra 1666MB

-------initial---------------------------------------------------------------
                 rss : 40.477 MB
           heapTotal : 6.746 MB
            heapUsed : 4.075 MB
            external : 0.364 MB
        arrayBuffers : 0.016 MB
-------deflated 2.116 MB-----------------------------------------------------
                 rss : 42.492 MB
           heapTotal : 5.996 MB
            heapUsed : 4.312 MB
            external : 2.479 MB
        arrayBuffers : 2.132 MB
-------inflated size 1598.928 MB---------------------------------------------
                 rss : 3269.332 MB
           heapTotal : 10.246 MB
            heapUsed : 4.249 MB
            external : 3200.344 MB
        arrayBuffers : 1601.060 MB
-------inflated=null---------------------------------------------------------
                 rss : 1669.563 MB
           heapTotal : 9.246 MB
            heapUsed : 4.234 MB
            external : 1601.407 MB
        arrayBuffers : 2.132 MB
-------deflated=null---------------------------------------------------------
                 rss : 1666.797 MB
           heapTotal : 6.246 MB
            heapUsed : 4.663 MB
            external : 2.479 MB
        arrayBuffers : 0.016 MB
-------after 10 seconds------------------------------------------------------
                 rss : 1666.582 MB
           heapTotal : 5.996 MB
            heapUsed : 4.281 MB
            external : 0.363 MB
        arrayBuffers : 0.016 MB

Additional information

The binary file is the deflated data chunk of a 400 mega pixel PNG

@devsnek
Copy link
Member

devsnek commented Sep 22, 2022

gc() by itself does not trigger a full garbage collection in v8. Additionally, the buffers are still in scope when you try to collect them so v8 is unlikely to assume they are unreachable at that point. I don't think there's any indication of a leak here.

@nitram-work nitram-work changed the title Zlib memory leak for inflate and inflateSync Zlib reserving twice the needed memory for inflate and inflateSync, kept in RSS until process exits. Sep 22, 2022
@nitram-work
Copy link
Author

Hi devsnek. You're correct it's not a leak. I've changed the title and description.

But something it is doing is reserving twice the memory needed, which for big images it can be huge and that's causing OOM kills in some scenarios on our containers.

@nitram-work
Copy link
Author

I don't understand what you meant about buffers in scope. Their refs are nulled before GC.

inflated = null;
deflated = null;

@nitram-work
Copy link
Author

Just tried with an image double the size and RSS also reserves double (6.5GB on inflate, then keeps 3.2GB in RSS)

inflate-2.zip

@nitram-work
Copy link
Author

nitram-work commented Sep 22, 2022

Ok, this issue was founded on my misunderstanding and misconception of the inner workings of the buffering.
I read node's source and it just:
Stores all chunks in an array
And concats them at the end

So it makes sense it temporarily reserves double the size. It might be a good idea to mention it in the docs that it takes 2x.

I'm closing the issue. Sorry for the troubles!

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

No branches or pull requests

2 participants