zlib: do not leak on destroy by mafintosh · Pull Request #23734 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Currently zlib streams will leak their handles if the streams are destroyed using .destroy(), which means they'll leak when used with stream.pipeline or pump and an error happens.

This PR fixes that by implementing _destroy and call _close on the handle always.

@mafintosh build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1283/pipeline

Do you know when this regressed? Or has it always done this? I am wondering if @nodejs/lts will need to backport it.

I guess there is no way to unit test this, but a manual test in the PR that could be run by hand while watching sys memory use would help in verifying backports.

@sam-github as far as I can see this have been an issue since Node 8 (previous to that there were no destroy() function on zlib streams)

/to @nodejs/lts

Currently zlib streams will leak their handles if the streams are destroyed using .destroy(), which means they'll leak when used with stream.pipeline or pump and an error happens.

That seems somewhat surprising… if it’s true, then this is possibly not a fix for that bug. Even without the extra _close() call here, zlib handles should be weak when not currently performing write operations, so they should be garbage-collected along with the rest of the stream.

@addaleax oh interesting, so the _close call is not necessary in general? my leak stats my be off

@mafintosh So… it is not strictly necessary in general, but it will release resources earlier than GC would, which is usually desirable I guess? So, if there are some long-lived references to the zlib stream, that could still be a memory leak – but the fix for that situation might be to not keep those references around, so that GC can do its work?

(i.e. this PR is definitely improving things, but you may want to look out for deeper-lying issues.)

There might also be a behavioural difference between v8.x and v10.x, at least currently. #21608 made memory allocation reporting for the native zlib handles more accurate, so GC may see lower or higher memory pressure than before.

There is already a check for this a few line above, isn't that one sufficient?

I guess the idea is that the push() call can trigger on('data') listeners which can destroy the stream?

Either way, it would be great to have a test for this (and the other bits in this PR)

Ok, makes sense, I think a comment would also help.

This is actually already tested as the existing tests fail (crash even!) without this.

@addaleax thanks for the explanation, let me see if I can cook up a test case based on that.

(I’m adding blocked because this conflicts with #23785 – without really voicing a preference, neither should land without deciding on that one specifically being the “better one”)

Added a test. @addaleax coordinated with @mcollina and we tested that his async iterator fix works on top of this one, so this should land first, then we'll rebase #23785 on top.

Confirming what @mafintosh said. This can land.

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/18042/

@mafintosh I'll land the async iterators fix as part of the other PR, can you remove it out from this one?

PR-URL: #23734 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This lands cleanly on 10.x but requires a backport for 8.x

Is it ready to land in LTS yet or does it need a bit more time to bake?

This is ready, but it will need a proper backport.

PR-URL: #23734 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

zlib: do not leak on destroy

f1055e3

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Oct 18, 2018

refack added the stream Issues and PRs related to the stream subsystem. label Oct 18, 2018

watson reviewed Oct 18, 2018

View reviewed changes

Comment thread lib/zlib.js

sam-github added lts-watch-v8.x labels Oct 18, 2018

addaleax removed the stream Issues and PRs related to the stream subsystem. label Oct 18, 2018

addaleax reviewed Oct 18, 2018

View reviewed changes

Comment thread lib/zlib.js

addaleax approved these changes Oct 18, 2018

View reviewed changes

jasnell approved these changes Oct 18, 2018

View reviewed changes

lpinca reviewed Oct 19, 2018

View reviewed changes

mafintosh mentioned this pull request Oct 20, 2018

Ensure async iteration works with destroyed streams #23785

Closed

3 tasks

lpinca approved these changes Oct 20, 2018

View reviewed changes

addaleax added the blocked PRs that are blocked by other issues or PRs. label Oct 21, 2018

add @mcollina 's test case

91cdb3d

mcollina approved these changes Oct 22, 2018

View reviewed changes

mcollina removed the blocked PRs that are blocked by other issues or PRs. label Oct 22, 2018

mafintosh deleted the do-not-leak-on-zlib-destroy branch October 23, 2018 16:06

targos mentioned this pull request Oct 27, 2018

Release proposal: v11.1.0 #23922

Merged

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Nov 26, 2018

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Nov 26, 2018

mcollina added the backport-requested-v8.x label Nov 26, 2018

codebytere added backported-to-v10.x and removed lts-watch-v10.x labels Dec 13, 2018

codebytere mentioned this pull request Jan 4, 2019

v10.15.1 proposal #25346

Merged

socket-security Bot mentioned this pull request May 9, 2026

build(deps): bump @babel/plugin-transform-modules-systemjs from 7.19.6 to 7.29.4 in /POCUploadStream/ClientApp in the npm_and_yarn group across 1 directory GuilhermeStracini/POC-react-dotnet-UploadStream#394

Merged

                   assert(false, 'have should not go down');
                 }
+                if (self.destroyed) {

zlib: do not leak on destroy by mafintosh · Pull Request #23734 · nodejs/node · GitHub

Uh oh!

Uh oh!

zlib: do not leak on destroy - #23734

zlib: do not leak on destroy#23734mafintosh wants to merge 4 commits intonodejs:masternodejs/node:masterfrom mafintosh:do-not-leak-on-zlib-destroymafintosh/io.js:do-not-leak-on-zlib-destroyCopy head branch name to clipboard

Conversation

mafintosh commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Oct 18, 2018

Uh oh!

sam-github commented Oct 18, 2018

Uh oh!

Uh oh!

watson commented Oct 18, 2018

Uh oh!

sam-github commented Oct 18, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mafintosh commented Oct 18, 2018

Uh oh!

addaleax commented Oct 18, 2018

Uh oh!

lpinca Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

lpinca Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

mafintosh Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

mafintosh commented Oct 19, 2018

Uh oh!

addaleax commented Oct 21, 2018

Uh oh!

mafintosh commented Oct 22, 2018

Uh oh!

mcollina commented Oct 22, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 22, 2018

Uh oh!

mcollina commented Oct 22, 2018

Uh oh!

MylesBorins commented Nov 26, 2018

Uh oh!

mcollina commented Nov 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

zlib: do not leak on destroy#23734
mafintosh wants to merge 4 commits into
nodejs:masterfrom
mafintosh:do-not-leak-on-zlib-destroy

mafintosh commented Oct 18, 2018 •
edited
Loading