◐ Shell
reader mode source ↗
Skip to content

zlib: do not leak on destroy#23734

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

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

Conversation

@mafintosh

@mafintosh mafintosh commented Oct 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Member
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.

@nodejs-github-bot

nodejs-github-bot commented Oct 18, 2018

Copy link
Copy Markdown
Collaborator

@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

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

sam-github commented Oct 18, 2018

Copy link
Copy Markdown
Contributor

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.

@watson

watson commented Oct 18, 2018

Copy link
Copy Markdown
Member

@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)

@sam-github

sam-github commented Oct 18, 2018

Copy link
Copy Markdown
Contributor

/to @nodejs/lts

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

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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.

@mafintosh

mafintosh commented Oct 18, 2018

Copy link
Copy Markdown
Member Author

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

@addaleax

addaleax commented Oct 18, 2018

Copy link
Copy Markdown
Member

@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.

@mafintosh

mafintosh commented Oct 19, 2018

Copy link
Copy Markdown
Member Author

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

@mafintosh mafintosh mentioned this pull request Oct 20, 2018
Closed
3 tasks
@addaleax

addaleax commented Oct 21, 2018

Copy link
Copy Markdown
Member

(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”)

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

mafintosh commented Oct 22, 2018

Copy link
Copy Markdown
Member Author

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.

@mcollina

mcollina commented Oct 22, 2018

Copy link
Copy Markdown
Member

Confirming what @mafintosh said. This can land.

mcollina
mcollina approved these changes Oct 22, 2018
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM

mcollina
mcollina approved these changes Oct 22, 2018
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM

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

mcollina commented Oct 22, 2018

Copy link
Copy Markdown
Member

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

@mcollina

mcollina commented Oct 22, 2018

Copy link
Copy Markdown
Member

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

21 hidden items Load more…
@mafintosh mafintosh deleted the do-not-leak-on-zlib-destroy branch October 23, 2018 16:06
mafintosh added a commit that referenced this pull request Oct 23, 2018
@mafintosh
zlib: do not leak on destroy …
8a02d94 
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>
targos pushed a commit that referenced this pull request Oct 24, 2018
@mafintosh @targos
zlib: do not leak on destroy …
b07cb48 
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>
@targos targos mentioned this pull request Oct 27, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed
@MylesBorins MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Nov 26, 2018
@MylesBorins

MylesBorins commented Nov 26, 2018

Copy link
Copy Markdown
Contributor

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?

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

mcollina commented Nov 26, 2018

Copy link
Copy Markdown
Member

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

codebytere pushed a commit that referenced this pull request Dec 13, 2018
@mafintosh @codebytere
zlib: do not leak on destroy …
22ee1e3 
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>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
@mafintosh @MylesBorins
zlib: do not leak on destroy …
c6a43fa 
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>
@codebytere codebytere mentioned this pull request Jan 4, 2019
Merged
This was referenced Jan 29, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@socket-security socket-security Bot mentioned this pull request May 9, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

+1 more reviewer

@watson watson watson left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready zlib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.