http: fixes memory retention issue with FreeList and HTTPParser

[leidegre]

Fixes: #29394
Refs: #33167 (comment)

The issue here is that because the HTTPParser is pooled it has the possibility to act as a retainer of a lot of data. In summary we have to reset parser.onIncoming and parser[kOnTimeout] to null to prevent these from retaining memory while the HTTPParser is sitting in the FreeList unused.

I'd like this to eventually be cherry picked into the Node 14 release.

See the reference for details.

---

@ronag Here's your PR.

[leidegre]

Adding this for clarification.

This isn't a strict memory leak issue. There's an upper limit to the free list of a 1000 entries, which means that at most, it will retain up to 1000 times whatever the amount of bytes retained by event handlers. In our case this was about 1.5 MB (our heap usage eventually grew to about 1.5 GB as the free list is filled up when the site is under heavier load, this takes several days to happen).

This issue has resulted in production outages due to memory starvation and I'm very eager to get this fixed.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31129/ ✅

[Trott]

Not a blocker to landing this, but is it possible to add a test?

[Trott]

@nodejs/http

[Trott]

@leidegre Thanks for all the work you did to track this down!

[leidegre]

@Trott Thanks.

I understand the desire to add a test but I'm not sure how I would go about that in a meaningful manner given the particulars of the code changes. It would be a lot of work for me to make a meaningful contribution.

I would be happy to verify any commits/builds though.

[lpinca]

@leidegre it should be sufficient to verify that parser.onIncoming is null after that the parser object is returned to the pool. There could already be a test that does something similar to use as a reference.

Anyway it can be added also in follow up PR.

[lpinca]

@leidegre feel free to add the following test under test/parallel

'use strict';

const common = require('../common');
const assert = require('assert');
const http = require('http');
const { HTTPParser } = require('_http_common');

// Test that the `HTTPParser` instance is cleaned up before being returned to
// the pool to avoid memory retention issues.

const kOnTimeout = HTTPParser.kOnTimeout | 0;
const server = http.createServer();

server.on('request', common.mustCall((request, response) => {
  const parser = request.socket.parser;

  assert.strictEqual(typeof parser[kOnTimeout], 'function');

  request.socket.on('close', common.mustCall(() => {
    assert.strictEqual(parser[kOnTimeout], null);
  }));

  response.end();
  server.close();
}));

server.listen(common.mustCall(() => {
  const request = http.get({ port: server.address().port });
  let parser;

  request.on('socket', common.mustCall(() => {
    parser = request.parser;
    assert.strictEqual(typeof parser.onIncoming, 'function');
  }));

  request.on('response', common.mustCall((response) => {
    response.resume();
    response.on('end', common.mustCall(() => {
      assert.strictEqual(parser.onIncoming, null);
    }));
  }));
}));

[leidegre]

@lpinca Thanks! I've included the test in the PR (I've amended this commit). I've also verified that the test fails with Node 14 and that it succeeds with these changes applied.

The test is here test/parallel/test-http-parser-memory-retention.js.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31144/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31161/

[leidegre]

This is currently an issue with Node 12 (as well as 14) so I was wondering what the process is for landing this in 12 (given that it's the current LTS) and that 14 will become the next LTS? Do I need to do something for this to happen or will this be added to some list and integrated into the next release?

[leidegre]

@addaleax Mind reader! 👍

[addaleax]

Do I need to do something for this to happen or will this be added to some list and integrated into the next release?

Commits are backported to LTS by default, so unless there are merge conflicts, no. If there are merge conflicts, you will be pinged, though. Also, just so you know, as a rule commits have to be released for 2 weeks in a Current (i.e. 14.x) release before they are backported to LTS release lines.

I’ve added the lts-watch label, but that’s more in order to make sure that this is taken into consideration and to explicitly mark this as something that should land on lts-v12.x.

@addaleax Mind reader!

I just saw your comment, that’s all 😄

[leidegre]

Looks like some of the checks have failed with some transient Git issue? I don't know if I can do anything about it...

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31176/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31177/ ✅

[leidegre]

Everything good now?

[Flarna]

Landed in 26f1500

[codebytere]

Backport currently blocked on #32329.

[leidegre]

@codebytere This is important to me. Is there anything I can do to unblock this?

[codebytere]

@leidegre if you're willing, this could be backported together with #32329

[Flarna]

Removed backport-blocked label because #32329 has been backported via #34131