http_parser: don't reuse HTTPParser

As discussed in nodejs/diagnostics#248 and
nodejs#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.

Author: mmarchini

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) => {