http: free parser on keepalive

[ronag]

Free parser when putting socket back in the agent keep alive free list.

Fixes: #29394

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ronag]

This is the significant change.

[ronag]

@leidegre

[leidegre]

@ronag unfortunately the problem is still there...

Looks like the problem is connected to onIncoming. It is holding on to the server and socket. The socket in turn is holding on to the _httpResponse which has an event handler that is holding on to the data.

Now, I have some concerns that the free list is running the constructor when the HTTPParser is allocated from the free list and not when it is put back into the free list for reuse. parsersCb in _http_common.js actually does clear onIncoming but this code is being run when the parser is recycled, not when it is finished, i.e. returned to the pool. This means that as long as the HTTPParser isn't reused it will retain data. It should release all its references when it is put back into the pool.

I'm also confused by why the code below isn't merged with cleanParser.

parser.onIncoming = null;
parser[kOnHeaders] = parserOnHeaders;
parser[kOnHeadersComplete] = parserOnHeadersComplete;
parser[kOnBody] = parserOnBody;
parser[kOnMessageComplete] = parserOnMessageComplete;
parser[kOnTimeout] = null;

After adding the parser.onIncoming = null; I get this:

Similar situation but different retainer. In this case it's the onParserTimeout function in _http_server.js. So I added parser[kOnTimeout] = null; as well to the cleanParser function and now finally, the memory is not being retained.

Echoing my original comment about whether it is truly meaningful to pool the HTTPParser. There's a lot of complexity here that goes away if we do not pool HTTPParser objects.

In summary, the memory leak goes away if we add these two lines to the cleanParser function.

+  parser[kOnTimeout] = null;
+  parser.onIncoming = null;

[ronag]

@leidegre Good digging there. I would add that comment to the original issue. I would not object to remove the pooling but I'm not sure about its performance implications.

[ronag]

@nodejs/http This PR does not solve the references issue. Is it still worth trying to land or should we close it?

[ronag]

In summary, the memory leak goes away if we add these two lines to the cleanParser function.

@leidegre Would you mind opening a PR with this change?

[leidegre]

@ronag I'll open a PR. I think I did mention it in my first comment but I can reiterate my fidings here the as well put emphasis on whether the pooling is necessary.