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.

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

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

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

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

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.

@dvfeinblum
Copy link

dvfeinblum commented Mar 27, 2017

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.

@joegoldbeck
Copy link

joegoldbeck commented Mar 28, 2017

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

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?

@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

@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
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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 29, 2017
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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 29, 2017
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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@stefanosala
Copy link
Author

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

MylesBorins pushed a commit that referenced this issue Mar 29, 2017
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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 29, 2017
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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 29, 2017
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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
kevinsawicki pushed a commit to electron/node that referenced this issue May 16, 2017
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 <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
memory Issues and PRs related to the memory management or memory footprint.
Projects
None yet
Development

No branches or pull requests