-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Node v12.16.2 memory issues after upgrade from v8 (<--- Last few GCs --->) #33266
Comments
/cc @mmarchini who I think was chasing down some other similar issues |
Thank you @RobertDittmann for the comprehensive report. I don't have bandwidth to investigate this right now, but hopefully someone else can take a look. One thing that might be helpful is providing a Node.js client instead of a Java client, since many of us might not have Java installed. You're seeing a similar issue on v12.13, is that correct? If that's the case, I wonder if we have an actual leak in core instead of a GC bug on V8. One thing that might be helpful (for anyone looking into this issue) is taking a heap profile of the application on different Node.js versions, to confirm that the allocation patterns are the same or if there's actually extra allocations happening). A heap snapshot might also be useful to show which objects are leaking (if that's the case). |
Correct. v12.13 was deployed on production with same issue. 1st graph from #32737
If I will have time to prepare Node.js client next week will provide it as well. |
I was able to run locally by the way, the heap profiler seems like a good start (the results on 10 and 12 are very different) |
It seems like the leak is coming from getObjectFromJson. If you comment out the body on that function memory doesn't seem to grow uncontrollably. I don't know why it's happening though, running just that function on a loop doesn't leak memory. (edit: nevermind, I was running on 10 by mistake 🤦) |
@mmarchini very good point !! Indeed I did tests with commenting out mentioned method body and results are (stable heap): But still do not know why for same source code memory usage is different between v8, v10 and v12. Method getObjectFromJson converts stringified json to object when possible or returns already ready to go object. This method is executed 2 times in given example. That is why I did another one test with grpc call modification:
It seems that there could be bug in:
We have several dozen of Node.js services and for some of them impact is bigger for others could be not visible yet. We experienced it after changing Node.js version as mentioned before. |
I've got something similar to this one, but mostly on node 14 (14.0.0 and 14.2.0). Usually RSS grows up to 512Mb and after that it remains on the same level for a long time. Also tried to add "--optimize-for-size" and other GC related stuff - it doesn't help. However downgrading node to 12.14.0 this rss growth is less. And for the same app under the same load rss stabilize at 200Mb. Created simple test here (with node 12.12 and node 14): On linux I have flowing results: |
It looks that memory leak is with cases similar to:
For:
and
seems to be fine. |
@SkeLLLa that sounds like a different issue. Do you mind opening a new issue in the repo to track that? @RobertDittmann the issue is likely some edge case, and not general JSON.parse or try/catch usage. Trying to parse an object might be the edge case. Either way, this does sound like an issue on V8, and we should report it upstream once we get a more narrow reproducible. |
I see that for other services where also memory issue exists there is no JSON parsing operation etc., so it seems that there is something wrong with way how V8 works. |
We downgraded to Node v10.20.1 for now. |
@wdittmer-mp because it is not fixable at this time or ? Is this just Node 12 related ? |
Starting from v12 (I think we also saw it with v13) we see this issue. As a temporary workaround we found that v10.20.1 has no issues and downgraded to that, so that at least we have a LTS version and production is stable. |
@MikevPeeren it is Node 12 related (did not test other versions cause we use LTS). Services are infected with poor memory handling. GC crashes services. Just for comparison we did loadtests on same service with node 10 and 12 and results are like below. V12 - 2 days running (services restarted then memory drop and again the same issue): |
@RobertDittmann Thanks for the great comparison, is there any news about this from Node itself or even acknowledgement on it ? I assume they want to fix this. |
@MikevPeeren @wdittmer-mp would be good to have reproducibles for your issues as well, to make sure they are the same as this one (and not a different memory issue).
Interesting, it might be something more subtle then (like having a builtin call inside try/catch on some very specific situations).
Best we can tell so far this seems like an issue on V8, not Node.js. The reproducible shared by @RobertDittmann is great for us to analyse, but it's too big to share with the V8 team, so we need to find a smaller one. |
@mmarchini I created ticket for V8 and I am waiting for their feedback. https://bugs.chromium.org/p/v8/issues/detail?id=10538 |
Great! It might be worth sharing a bit more context there. A heap profile and a heap snapshot, for example. Maybe also the output from |
@RobertDittmann is my colleague working on the same project 😅, hence no other repro. |
@mmarchini I updated both repositories. You can pull changes. Now it is possible to run both versions: previous one with grpc, queue and decorators and new test is with simple REST call. Steps to reproduce issue with REST test:
Hope now it is "simple" enough :) |
@RobertDittmann great! Using a simpler stack is good. Any chance you can replace the Java service with an HTTP load tester like autocannon? I'm also wondering if we can speed up this test a bit (maybe increase the RPS on the client, if the server isn't saturated), 36 minutes is a long time for a test, especially when comparing different Node.js versions. |
I did small changes in java and node service (pull required). You can run node with command:
After 3 minutes it will rach 128 - then 6-7 minutes to failure (5-6 times faster) - no need to set memory to 384. It is handling around 5k requests with 200 status (per second). I will check autocannon tomorrow. Some graphs from today: Now memory clearing. Both cases are similar. Around 2nd minute stopped java service and started around 4th minute and then again stopped at 5th.
|
Thanks, I'll try it today. I was hoping we could make the test grow memory faster instead of crashing faster by reducing the heap size. Growing memory faster gives us relevant information we can share with V8 faster, whereas crashing doesn't give much information we can use. I'll try it today with the most recent changes and then share the results with V8. |
It grows faster. You have 128MB within 3 minutes not 14. |
According to comment from https://bugs.chromium.org/p/v8/issues/detail?id=10538#c7:
Any info if this is or can be part of LTS version and which ? |
Will need to evaluate if the changes are backportable, but I think it's worth trying, yes. Good thing they found it! I just came up with an isolated test case and would share there, but apparently they beat me to it :D |
Ok, seems to be fixed on v14, so we only need to backport to v12. |
Any news with the backporting, I'd love to see the RSS drop in my node process. |
@lundibundi Alright, I will wait for the v12 release to update my node version. There isn't any other quick way in the meantime, is there? |
@Golorio this is a very specific leak, so it would be good to confirm if you're experiencing it or something else. The cause is described here:
In other words, every time If you're not sure if that's the same leak, I suggest opening a new issue filling the template, so folks can help investigate. Or, if you want to wait for the next release to try it, the ETA is to have a release candidate next week and the actual release two weeks from now. |
@mmarchini I see, thank you very much. I do believe my issue is connected to this one since I do use JSON.parse extensively in my code, have an rss leak, are in node v12 and if I remember correctly the issue started after upgrading from v10 although now I can't downgrade. |
Unfortunately, it seems this was not added on v12.18.1. I'll wait for v12.18.2. Regardless, good work on the updates. |
The fix is included in v12.18.2. @RobertDittmann let us know if this release fixes your memory leak. I'll close the issue now, if anyone else is still experiencing memory leaks on v12.18.2, I'd recommend opening another issue since it's probably a different memory leak (feel free to ping me in that case). |
Hi it seems that it works fine after fix. Thanks ! |
This issue is continuation of #32737
Version:
v12.16.2 (but v12.13 on production)
Platform:
Darwin Kernel Version 19.4.0: Wed Mar 4 22:28:40 PST 2020; root:xnu-6153.101.6~15/RELEASE_X86_64 x86_64
(but docker with node:12.13.0-alpine on production)
Subsystem:
? runtime, heap, garbage collection
Description:
As in previous ticket: "We recently upgrade our production servers with docker containers with node v8 to docker containers with node v12.10 (node:12.13.0-alpine). At first all seems fine, but then we started noticing pod restarts by Kubernetes being OOM Killed. Since the upgrade, memory usage seems to increase over time sometimes in steep inclines until reaching ~500MB at which time they are killed by Kuberenetes."
With the same code base and dependencies, when switching between 3 versions of node (8.17.0, 10.20.1, 12.16.2) different memory usage observed. With version 12.16.2 node service crashes with logs:
What steps will reproduce the bug?
Same situation appears on production environment but it takes few days to happen.
Below comparison of node vesions:
node v12.16.2 started with command: node --max-old-space-size=384 app.js (crashed - results as above logs)
node v12.16.2 started with command: node app.js
node v10.20.1 started with command: node app.js (it shows also memory when load stopped)
node v8.17.0 started with command: node app.js
How often does it reproduce? Is there a required condition?
Always.
What is the expected behavior?
A stable heapUsed like in v8.17 and no spikes in memory usage causing OOM kills/ GCs issues.
What do you see instead?
Memory increase and GCs issues.
Additional information
I am looking for solutions. Seems that used last years services cannot be used with LTS v12 on production.
Please let me know how I can help further,
Kind regards,
Robert
The text was updated successfully, but these errors were encountered: