http_parser: don't reuse HTTPParser

As discussed in nodejs/diagnostics#248 and
#21313, reusing HTTPParser resource
is a blocker for landing
`require('async_hooks').currentAsyncResource()`, since reusing an async
resource interanlly would make it infeasible to use them reliably in
WeakMaps. Two suggestions came up: have a wrapper around the HTTPParser
which we would use as the async resource, or stop reusing HTTPParser in
our HTTP server/client code.

This commit implements the latter, since we have a better GC now and
reusing HTTPParser might make sense anymore. This also will avoid one
extra JS->C++ call in some cases, which should improve performance for
these cases.

benchmark/http/bench-parser.js

@@ -19,12 +19,10 @@ function main({ len, n }) {
  const CRLF = '\r\n';
 
  function processHeader(header, n) {
- const parser = newParser(REQUEST);
-
  bench.start();
  for (var i = 0; i < n; i++) {
+ const parser = newParser(REQUEST);
  parser.execute(header, 0, header.length);
- parser.reinitialize(REQUEST, i > 0);
  }
  bench.end(n);
  }

lib/_http_client.js

@@ -29,9 +29,9 @@ const assert = require('assert').ok;
 const {
  _checkIsHttpToken: checkIsHttpToken,
  debug,
+ createParser,
  freeParser,
- httpSocketSetup,
- parsers
+ httpSocketSetup
 } = require('_http_common');
 const { OutgoingMessage } = require('_http_outgoing');
 const Agent = require('_http_agent');
@@ -47,7 +47,6 @@ const {
  ERR_UNESCAPED_CHARACTERS
 } = require('internal/errors').codes;
 const { validateTimerDuration } = require('internal/timers');
-const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
 
 const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/;
 
@@ -629,10 +628,9 @@ function emitFreeNT(socket) {
 }
 
 function tickOnSocket(req, socket) {
- var parser = parsers.alloc();
+ const parser = createParser(HTTPParser.RESPONSE);
  req.socket = socket;
  req.connection = socket;
- parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]);
  parser.socket = socket;
  parser.outgoing = req;
  req.parser = parser;

lib/_http_common.js

@@ -23,7 +23,6 @@
 
 const { methods, HTTPParser } = internalBinding('http_parser');
 
-const { FreeList } = require('internal/freelist');
 const { ondrain } = require('internal/http');
 const incoming = require('_http_incoming');
 const {
@@ -147,9 +146,8 @@ function parserOnMessageComplete() {
  readStart(parser.socket);
 }
 
-
-const parsers = new FreeList('parsers', 1000, function parsersCb() {
- const parser = new HTTPParser(HTTPParser.REQUEST);
+function createParser(type) {
+ const parser = new HTTPParser(type);
 
  cleanParser(parser);
 
@@ -160,31 +158,21 @@ const parsers = new FreeList('parsers', 1000, function parsersCb() {
  parser[kOnMessageComplete] = parserOnMessageComplete;
 
  return parser;
-});
+}
 
 function closeParserInstance(parser) { parser.close(); }
 
 // Free the parser and also break any links that it
 // might have to any other things.
 // TODO: All parser data should be attached to a
 // single object, so that it can be easily cleaned
-// up by doing `parser.data = {}`, which should
-// be done in FreeList.free. `parsers.free(parser)`
-// should be all that is needed.
+// up by doing `parser.data = {}`.
 function freeParser(parser, req, socket) {
  if (parser) {
  if (parser._consumed)
  parser.unconsume();
  cleanParser(parser);
- if (parsers.free(parser) === false) {
- // Make sure the parser's stack has unwound before deleting the
- // corresponding C++ object through .close().
- setImmediate(closeParserInstance, parser);
- } else {
- // Since the Parser destructor isn't going to run the destroy() callbacks
- // it needs to be triggered manually.
- parser.free();
- }
+ setImmediate(closeParserInstance, parser);
  }
  if (req) {
  req.parser = null;
@@ -239,9 +227,9 @@ module.exports = {
  continueExpression: /(?:^|\W)100-continue(?:$|\W)/i,
  CRLF: '\r\n',
  debug,
+ createParser,
  freeParser,
  httpSocketSetup,
  methods,
- parsers,
  kIncomingMessage
 };

lib/_http_server.js

@@ -26,7 +26,7 @@ const net = require('net');
 const { HTTPParser } = internalBinding('http_parser');
 const assert = require('assert').ok;
 const {
- parsers,
+ createParser,
  freeParser,
  debug,
  CRLF,
@@ -42,7 +42,6 @@ const {
  defaultTriggerAsyncIdScope,
  getOrSetAsyncId
 } = require('internal/async_hooks');
-const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
 const { IncomingMessage } = require('_http_incoming');
 const {
  ERR_HTTP_HEADERS_SENT,
@@ -338,8 +337,7 @@ function connectionListenerInternal(server, socket) {
  socket.setTimeout(server.timeout);
  socket.on('timeout', socketOnTimeout);
 
- var parser = parsers.alloc();
- parser.reinitialize(HTTPParser.REQUEST, parser[is_reused_symbol]);
+ const parser = createParser(HTTPParser.REQUEST);
  parser.socket = socket;
  socket.parser = parser;
 

src/node_http_parser.cc

@@ -494,30 +494,6 @@ class Parser : public AsyncWrap, public StreamListener {
  }
 
 
- static void Reinitialize(const FunctionCallbackInfo<Value>& args) {
- Environment* env = Environment::GetCurrent(args);
-
- CHECK(args[0]->IsInt32());
- CHECK(args[1]->IsBoolean());
- bool isReused = args[1]->IsTrue();
- parser_type_t type =
- static_cast<parser_type_t>(args[0].As<Int32>()->Value());
-
- CHECK(type == HTTP_REQUEST || type == HTTP_RESPONSE);
- Parser* parser;
- ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
- // Should always be called from the same context.
- CHECK_EQ(env, parser->env());
- // This parser has either just been created or it is being reused.
- // We must only call AsyncReset for the latter case, because AsyncReset has
- // already been called via the constructor for the former case.
- if (isReused) {
- parser->AsyncReset();
- }
- parser->Init(type);
- }
-
-
  template <bool should_pause>
  static void Pause(const FunctionCallbackInfo<Value>& args) {
  Environment* env = Environment::GetCurrent(args);
@@ -923,7 +899,6 @@ void Initialize(Local<Object> target,
  env->SetProtoMethod(t, "free", Parser::Free);
  env->SetProtoMethod(t, "execute", Parser::Execute);
  env->SetProtoMethod(t, "finish", Parser::Finish);
- env->SetProtoMethod(t, "reinitialize", Parser::Reinitialize);
  env->SetProtoMethod(t, "pause", Parser::Pause<true>);
  env->SetProtoMethod(t, "resume", Parser::Pause<false>);
  env->SetProtoMethod(t, "consume", Parser::Consume);

test/parallel/test-http-parser-free.js

@@ -41,7 +41,7 @@ server.listen(0, function() {
  if (!parser) {
  parser = req.parser;
  } else {
- assert.strictEqual(req.parser, parser);
+ assert.notStrictEqual(req.parser, parser);
  }
 
  countdown.dec();

test/parallel/test-http-parser.js

@@ -98,8 +98,6 @@ function expectBody(expected) {
  throw new Error('hello world');
  };
 
- parser.reinitialize(HTTPParser.REQUEST, false);
-
  assert.throws(
  () => { parser.execute(request, 0, request.length); },
  { name: 'Error', message: 'hello world' }
@@ -558,10 +556,10 @@ function expectBody(expected) {
  parser[kOnBody] = expectBody('ping');
  parser.execute(req1, 0, req1.length);
 
- parser.reinitialize(REQUEST, false);
- parser[kOnBody] = expectBody('pong');
- parser[kOnHeadersComplete] = onHeadersComplete2;
- parser.execute(req2, 0, req2.length);
+ const parser2 = newParser(REQUEST);
+ parser2[kOnBody] = expectBody('pong');
+ parser2[kOnHeadersComplete] = onHeadersComplete2;
+ parser2.execute(req2, 0, req2.length);
 }
 
 // Test parser 'this' safety

test/sequential/test-http-regr-gh-2928.js

@@ -7,15 +7,14 @@ const common = require('../common');
 const assert = require('assert');
 const httpCommon = require('_http_common');
 const { internalBinding } = require('internal/test/binding');
-const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
 const { HTTPParser } = internalBinding('http_parser');
 const net = require('net');
 
-const COUNT = httpCommon.parsers.max + 1;
+const COUNT = 1;
 
 const parsers = new Array(COUNT);
 for (let i = 0; i < parsers.length; i++)
- parsers[i] = httpCommon.parsers.alloc();
+ parsers[i] = httpCommon.createParser(HTTPParser.RESPONSE);
 
 let gotRequests = 0;
 let gotResponses = 0;
@@ -26,7 +25,6 @@ function execAndClose() {
  process.stdout.write('.');
 
  const parser = parsers.pop();
- parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]);
 
  const socket = net.connect(common.PORT);
  socket.on('error', (e) => {