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

Keepalive prevents garbage collection on unread responses #29394

Closed
orgads opened this issue Sep 1, 2019 · 16 comments
Closed

Keepalive prevents garbage collection on unread responses #29394

orgads opened this issue Sep 1, 2019 · 16 comments
Labels
http Issues or PRs related to the http subsystem. memory Issues and PRs related to the memory management or memory footprint.

Comments

@orgads
Copy link
Contributor

orgads commented Sep 1, 2019

If keepAlive is enabled for the agent, and the response is not read by the client, the memory consumption grows very fast.

This is possibly similar (but not caused by) to recent fixes like #29297 and its predecessors.

const app = require('express')();
app.get('/test', (req, res) => res.sendFile(__filename));
app.listen(3000);

const http = require('http');
http.globalAgent.keepAlive = true;

setInterval(() => console.log(process.memoryUsage()), 1000);
setInterval(() => {
  http.get('http://127.0.0.1:3000/test', (res) => { /*res.resume();*/ });
}, 1);

If I uncomment res.resume() (or comment out keepAlive) then memory consumption is sane (around 120M). But when it is commented out, memory usage reaches 280M.

Using the inspector, I see that there are many stale Socket objects, which are retained by onIncoming in HTTPParser in FreeList. There are no application retainers. The application has no access to this IncomingMessage since it is not stored anywhere by the application, so it should be garbage collected eventually.

It looks like after a lot of time, this HTTPParser is reallocated for another request, and only then this response is GC'ed.

@orgads
Copy link
Contributor Author

orgads commented Sep 1, 2019

Related: #29118

@Fishrock123 Fishrock123 added the http Issues or PRs related to the http subsystem. label Sep 1, 2019
@ronag
Copy link
Member

ronag commented Sep 2, 2019

I'm not sure but we might be missing a:

const parser = socket.parser;   
if (parser) {     
  parser.finish();     
  freeParser(parser, req, socket);   
}

In a few places. Not sure if the parser should be freed before the socket is put into the agent pool through 'free'.

Possible places that need it (i.e. before either emit('free', socket) or socket.destroy():

responseOnEnd
responseKeepAlive
onSocketNT

@addaleax: How does parser allocation/deallocation work? Do we need to free the parser for:

A. destroyed sockets?
B. pooled/re-used sockets?

@orgads
Copy link
Contributor Author

orgads commented Sep 2, 2019

CC: @brandonstark, @yoshigev

@orgads
Copy link
Contributor Author

orgads commented Sep 4, 2019

Please add also memory label.

@orgads
Copy link
Contributor Author

orgads commented Sep 16, 2019

@addaleax @ronag ping?

@bashanam
Copy link

+1

@ronag
Copy link
Member

ronag commented Sep 16, 2019

@orgads: I'm sorry. I can't help until I get some further clarification myself.

@addaleax
Copy link
Member

@ronag The HTTP code as never written very cleanly, and it wouldn’t be surprising if something’s missing here.

I don’t think we need to free the parser for re-used sockets; afaik, it is built with the intention of being able to parse multiple HTTP messages on one socket in mind.

@orgads
Copy link
Contributor Author

orgads commented Oct 20, 2019

Any progress?

@leidegre
Copy link
Contributor

leidegre commented Apr 28, 2020

Is the added benefit of object pooling here really motivated given all the added complexity? I was able to trace this back to this commit (which is over 10 years old) but I could not find any motivation for why it was deemed necessary. Is object construction (in this particular case) so expensive that this actually has a tangible performance impact? V8 has changed so much since then that I have to assume that any assumptions made back then won't hold today. Would it be possible to just remove the object pooling altogether?

I stumbled into this because of a memory leak issue, and the FreeList show up as a retainer of a lot of stuff.

@ronag ronag added the memory Issues and PRs related to the memory management or memory footprint. label Apr 28, 2020
@leidegre
Copy link
Contributor

leidegre commented Apr 30, 2020

I'd like to share some additional details from one of our production environments. In our particular case there's a payload of about 1.5 MB that is being retained for each HTTPParser that is active in the FreeList. During increased load hours, we note that FreeList is growing up to capacity, i.e. 1000. This is why we can see that it tops out at 1.5 GB.

This problem is exacerbated in our case because the large payload is being retained in a Promise which in return is being retained by the ServerResponse which ultimately is being retained by the HTTPParser (in the FreeList).

The big drops in memory usage are restarts.

image

ronag added a commit to nxtedition/node that referenced this issue Apr 30, 2020
Free parser when putting socket back in the agent
keep alive free list.

Fixes: nodejs#29394
@ronag
Copy link
Member

ronag commented Apr 30, 2020

@leidegre @orgads Can you try building this branch and see if it resolves the issue? #33167

@leidegre
Copy link
Contributor

leidegre commented Apr 30, 2020

@ronag will give it a go.

Meanwhile, I've been trying to figure out why it's retaining so much data. It's a big app and the problem doesn't reproduce easily, I've tried various repos for a while now without any luck. The only thing I can tell from this picture is that somehow the majority of bytes are being retained in the FreeList. Though, this isn't at all trivial to reproduce so I cannot share this test.

image

* the red stuff is my code.

@leidegre
Copy link
Contributor

leidegre commented Apr 30, 2020

@ronag is it possible to load core modules from external file instead, since this is just a change in Node.js JavaScript core module... build everything from scratch is a bit of chore. Happy to do it but it's going to take some time.

Apparently there seem to be but it's a build flag? So I would need to build a special Node binary anyway... or it hasn't landed yet. I found the PR at least...

@ronag
Copy link
Member

ronag commented Apr 30, 2020

@ronag is it possible to load core modules from external file instead

No idea, not familiar with this.

Happy to do it but it's going to take some time.

I'm happy to wait 😄

@leidegre
Copy link
Contributor

leidegre commented May 1, 2020

We did some work on this and uncovered some stuff.

The are additional references parser.onIncoming and parser[kOnTimeout] that aren't being reset to null. These retain the socket, which in turn retain the _httpResponse and which in turn can retain callbacks that hold on to data.

There are two options here.

  1. Add parser.onIncoming = null and parser[kOnTimeout] = null to cleanParser.

  2. Remove all the code related to the pooling of the HTTPParser. This code has been in Node since forever and I do not for sure know that the pooling is really improving the situation. It adds considerable GC pressure and the code is quite complex because of everything that needs to be reset. I would like to try to remove it to see what happens.

@Flarna Flarna closed this as completed in 26f1500 May 5, 2020
codebytere pushed a commit that referenced this issue May 7, 2020
Fixes: #29394
Refs: #33167 (comment)

PR-URL: #33190
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Fixes: #29394
Refs: #33167 (comment)

PR-URL: #33190
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. memory Issues and PRs related to the memory management or memory footprint.
Projects
None yet
6 participants