Release zlib callbacks and buffer after processing chunks

[mdlavin]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• the commit message follows commit guidelines

Affected core subsystem(s)

zlib

Description of change

When a long lived zlib compressor is created, using zlib.createDeflateRaw() for example, the compressor will hold onto some functions and buffers longer than it needs to. If there are lots of zlib compressors created, the total allocated space can add up to consume a non-trivial amount of memory.

One example of a real project that can experience this issue is the WS library with per-message deflating: https://github.com/websockets/ws/blob/master/lib/PerMessageDeflate.js . A common user of that is socket.io, which will create possibly create many web sockets. If a socket.io server has lots of web sockets connected with zlib compression, the leaked objects can take memory that could be freed. When New Relic is being used, the amount of memory is even more obvious, because the held closures also keep some New Relic transaction data alive as well.

I know that test cases are desired for changes, but I wasn’t sure about the best way to automate the test for this bug. There are some simple tests that could be added, like checking for certain attributes being null after certain code execution, but I wasn’t sure if that was useful enough. I’m open to suggestions. In place of a unit test, I created a standalone example that can demonstrate the problem here: https://gist.github.com/mdlavin/334338eb801bd3243679db89c219c7ac

Before the fix, the output looks like this:

Closure changes [ { what: 'Closure',
    size_bytes: 648,
    size: '648 bytes',
    '+': 11,
    '-': 2 } ]
Uint8Array changes [ { what: 'Uint8Array',
    size_bytes: 240,
    size: '240 bytes',
    '+': 3,
    '-': 0 } ]

After the fix, the number of Closures and Uint8Arrays are reduced by 1 each because the memory can be reclaimed by the GC.

Closure changes [ { what: 'Closure',
    size_bytes: 576,
    size: '576 bytes',
    '+': 10,
    '-': 2 } ]
Uint8Array changes [ { what: 'Uint8Array',
    size_bytes: 160,
    size: '160 bytes',
    '+': 2,
    '-': 0 } ]

[bnoordhuis]

For efficiency reasons, it's better to set the values to null or undefined. The delete operator changes the shape (a.k.a. the hidden class) of the object.

[mdlavin]

Thanks for the feedback. I updated the PR with your suggestions. I was't sure if you preferred to commits to be squash or not, so I pushed a new commit. I'm happy to squash if that's desired.

[ronkorving]

Will this ever be falsy or not req?

[addaleax]

no and no, I think… I guess just asking to change the variable instead of asking for the above comment woud have been easier, sorry

[mdlavin]

this will be falsy when callback is called from line 547. And, this will equal newReq (not req) when callback is called from the async write on line 623.

Well, I guess req and newReq are technically the same value with the current implementation. But this can definitely be falsy

[addaleax]

@mdlavin newReq === req because _handle.write() just returns _handle (I know it’s confusing)… but yes you can leave it like this then.

[addaleax]

@mdlavin This will be squashed anyway when it’s landed, until then you can do it in whatever way you think helps best with reviewing.

CI: https://ci.nodejs.org/job/node-test-commit/3494/

[mdlavin]

@addaleax it looks like that integration build failed for reasons other than my change. Does that match what you see?

[addaleax]

Yes, the CI failures are unrelated, no need to worry about that.

LGTM

[jhamhader]

LGTM

[addaleax]

Landed in 313ef54, thanks! I believe this is your first commit to core, if so, welcome on board and we hope you find other ways to contribute!

[mdlavin]

@addaleax this is my first commit and I'm happy to finally contribute. I'd like to backport this fix to the 4.x series if you think it's a good candidate. The changes are very simple, and I think the change applies cleanly in the 4.x version of zlib too.

[addaleax]

@mdarse I think you can do a backport PR, yes, and I wouldn’t expect much trouble doing that. Make sure to take the commit that landed in master and create a PR with it against v4.x-staging; and btw, changes need to live a while in a released version in order to be eligible for inclusion into LTS, so don’t expect it to get merged right away. :)

[mdlavin]

Thanks for the pointers. I opened #7251 as the backport request