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

Suspect memory leak in v6.10.1 #12033

Closed
stefanosala opened this issue Mar 25, 2017 · 20 comments
Closed

Suspect memory leak in v6.10.1 #12033

stefanosala opened this issue Mar 25, 2017 · 20 comments
Labels
memory Issues and PRs related to the memory management or memory footprint.

Comments

@stefanosala
Copy link

  • Version: v6.10.1
  • Platform: Linux f227befc-7ce9-41a4-9b09-363eac11b499 3.13.0-112-generic Added DS_Store to gitignore #159-Ubuntu SMP Fri Mar 3 15:26:07 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem:

Hi there, we noticed a weird behaviour after upgrading to v6.10.1 from v6.10.0. Basically we see memory accumulating until Heroku restarts the process.

You can better notice the behaviour in this graph:

v1314 is the upgrade to v6.10.1, v1315 is the rollback to v6.10.0.

We start the process with this command: NODE_ICU_DATA=node_modules/full-icu node --max-old-space-size=900 lib/processes/oneInstance.js.

Help? :) Please let me know if and how we can provide more useful info.

Thanks a lot.
Stefano
@stefanosala
Copy link
Author

We had a similar behaviour when we tried node v7, I'm not sure if that helps :)

@vsemozhetbyt vsemozhetbyt added the memory Issues and PRs related to the memory management or memory footprint. label Mar 25, 2017
@mscdex mscdex added the v6.x label Mar 25, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Mar 25, 2017

@stefanosala do you have any code you can share?

@stefanosala
Copy link
Author

@cjihrig we haven't isolated the issue yet, I'm sorry :(

@joyeecheung
Copy link
Member

joyeecheung commented Mar 25, 2017 •
edited
Loading

You mentioned this behavior also presents in v7, can you provide the specific version of that release? Cross comparing it with #11759 we might be able to find the related commit.

@lpinca
Copy link
Member

lpinca commented Mar 25, 2017 •
edited
Loading

Possibly related: #12019.

@stefanosala
Copy link
Author

Yep, we had the same issue with v7.6.0.

@SuperTux88
Copy link

SuperTux88 commented Mar 26, 2017 •
edited
Loading

I have the same behaviour after upgrading to 6.10.1 with camo, started with this command: /usr/bin/node /srv/camo/node_modules/.bin/coffee server.coffee.

@rowanmanning rowanmanning mentioned this issue Mar 27, 2017
Closed
@dvfeinblum
Copy link

dvfeinblum commented Mar 27, 2017 •
edited
Loading

I also see similar issues. My child processes are utilizing more ram than they are provisioned (using the --max_old_space_size flag) and I'm seeing a linear increase in memory usage.

Edit: Bit more information. I am using child_process.exec() to fire off my workers and switching to 6.9 has fixed the problem. Memory is no longer increasing linearly and I'm only using 15gb as opposed to maxing out the 30 I have at my disposal. Scripts also seem to be respecting the 1.5 gb I'm giving each of them now.

@kmikiy
Copy link

kmikiy commented Mar 28, 2017

I've also had the same issue
6.10.0 works fine
6100 - lassitva -no date

6.10.1 starts leaking
6101 - lassitva - no date

7.5.0 also works fine
7050 - lassitva - no date

7.6.0 and up starts leaking
7060 - lassitva - no date

Same code was running all 4 times, only changed the node version.

@joyeecheung
Copy link
Member

Excluding benchmark, doc, meta, test, tools and build, these are the PRs both added in v6.10.1 and v7.6.0 (obtained by scripting the OP of #11185 and #11759, I might be missing something though..)

@MylesBorins
Copy link
Contributor

@joyeecheung thank you so much for compiling that list

@nodejs/lts @nodejs/collaborators do any of the above prs stand out to you?

@addaleax
Copy link
Member

src: unconsume stream fix in internal http impl #11015

Seems like the most likely candidate from the list by far … /cc @Kasher @jasnell @indutny

@addaleax
Copy link
Member

Maybe strike that, I’ve just seen #12089, that looks like a good candidate for fixing the leak.

@addaleax addaleax mentioned this issue Mar 28, 2017
Closed
2 tasks
@joegoldbeck
Copy link

joegoldbeck commented Mar 28, 2017 •
edited
Loading

We ran into a memory leak in 4.8.1 (was not there in 4.8.0). These may be related.

@shigeki
Copy link
Contributor

shigeki commented Mar 28, 2017

I confirmed that #12089 fixes a memory leak bug and I'm sure that it resolves this issue.

@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 28, 2017 •
edited
Loading

As it would appear that the memory leak is also appear on v4.x I've gone ahead and made a list of commits that appears in v4.8.1, v6.10.1, and v7.6.0. It is much smaller

edit: @nodejs/streams do you think #11015 could have caused a leak?

@MylesBorins MylesBorins mentioned this issue Mar 28, 2017
Closed
@shigeki
Copy link
Contributor

shigeki commented Mar 28, 2017

TLS and HTTP client memory leaks were fixed in a6f9494.
I'm sure it fixes this. Please test it.

I apology my bug in my commit and thanks for everyone helping and identifying the issues.

@stefanosala
Copy link
Author

stefanosala commented Mar 28, 2017 •
edited
Loading

@shigeki thanks a lot and thanks to everybody else involved, we appreciate a lot! :)

I'll test the new release asap.

@dvfeinblum
Copy link

Yeah, thanks a bunch for fixing this! We'll give this patch a go next time we run our job.

MylesBorins pushed a commit that referenced this issue Mar 28, 2017
@MylesBorins
crypto: fix memory leak if certificate is revoked
a64aa44 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 29, 2017
@MylesBorins
crypto: fix memory leak if certificate is revoked
1673112 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 29, 2017
@MylesBorins
crypto: fix memory leak if certificate is revoked
2010f43 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@stefanosala
Copy link
Author

Stats are looking good with v7.8.0, thanks again!

@satazor satazor mentioned this issue Mar 29, 2017
Open
MylesBorins pushed a commit that referenced this issue Mar 29, 2017
@MylesBorins
crypto: fix memory leak if certificate is revoked
0490736 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 29, 2017
@MylesBorins
crypto: fix memory leak if certificate is revoked
5f644d2 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 29, 2017
@MylesBorins
crypto: fix memory leak if certificate is revoked
9d7fba4 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gibfahn gibfahn mentioned this issue Mar 31, 2017
Closed
kevinsawicki pushed a commit to electron/node that referenced this issue May 16, 2017
@kevinsawicki
crypto: fix memory leak if certificate is revoked
9b1683e 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: nodejs/node#9469
Fixes: nodejs/node#12033
PR-URL: nodejs/node#12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
memory Issues and PRs related to the memory management or memory footprint.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests