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.

Sign up for GitHub

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

Jump to bottom

zlib: do not leak on destroy #23734

Closed
wants to merge 4 commits into from
Closed

zlib: do not leak on destroy #23734

wants to merge 4 commits into from

Conversation

mafintosh
Copy link
Member

@mafintosh mafintosh commented Oct 18, 2018 •
edited
Loading

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
Copy link
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

@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Oct 18, 2018
@refack refack added the stream Issues and PRs related to the stream subsystem. label Oct 18, 2018
@sam-github
Copy link
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 reviewed Oct 18, 2018
View reviewed changes
lib/zlib.js Show resolved Hide resolved
@watson
Copy link
Member

watson commented Oct 18, 2018

@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
Copy link
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
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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.

lib/zlib.js Show resolved Hide resolved
@mafintosh
Copy link
Member Author

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

@addaleax
Copy link
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.

lpinca
lpinca reviewed Oct 19, 2018
View reviewed changes
lib/zlib.js
@@ -592,6 +597,10 @@ function processCallback() {
assert(false, 'have should not go down');
}

if (self.destroyed) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@mafintosh
Copy link
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
Copy link
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
Copy link
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
Copy link
Member

Confirming what @mafintosh said. This can land.

mcollina
mcollina approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

mcollina
mcollina approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

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

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

@mcollina
Copy link
Member

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

@mafintosh mafintosh closed this Oct 23, 2018
@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
Copy link
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
Copy link
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@watson watson watson left review comments

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.