Keepalive prevents garbage collection on unread responses

[orgads]

• Version: v12.9.1
• Platform: Linux junior 5.2.0-2-amd64 deps: update openssl to 1.0.1j #1 SMP Debian 5.2.9-2 (2019-08-21) x86_64 GNU/Linux
• Subsystem: net

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]

Related: #29118

[ronag]

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]

CC: @brandonstark, @yoshigev

[orgads]

Please add also memory label.

[orgads]

@addaleax @ronag ping?

[bashanam]

+1

[ronag]

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

[addaleax]

@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]

Any progress?

[leidegre]

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.

[leidegre]

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.

[ronag]

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

[leidegre]

@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.

* the red stuff is my code.

[leidegre]

@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]

@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]

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.