http: remove duplicate async_hooks init for http parser

Each time a new parser was created, AsyncReset was being called via
the C++ Parser class constructor (super constructor AsyncWrap) and also
via Parser::Reinitialize.

This also adds a missing async_hooks destroy event before AsyncReset is
called for the parser reuse case, otherwise the old async_id never gets
destroyed.

Refs: nodejs#19859

Author: basti1302

benchmark/http/bench-parser.js

@@ -25,7 +25,7 @@ function main({ len, n }) {
     bench.start();
     for (var i = 0; i < n; i++) {
       parser.execute(header, 0, header.length);
-      parser.reinitialize(REQUEST);
+      parser.reinitialize(REQUEST, i > 0);
     }
     bench.end(n);
   }

lib/_http_client.js

@@ -36,7 +36,11 @@ const {
 const { OutgoingMessage } = require('_http_outgoing');
 const Agent = require('_http_agent');
 const { Buffer } = require('buffer');
-const { defaultTriggerAsyncIdScope } = require('internal/async_hooks');
+const {
+  defaultTriggerAsyncIdScope,
+  destroyHooksExist,
+  emitDestroy
+} = require('internal/async_hooks');
 const { URL, urlToOptions, searchParamsSymbol } = require('internal/url');
 const { outHeadersKey, ondrain } = require('internal/http');
 const {
@@ -631,7 +635,10 @@ function tickOnSocket(req, socket) {
   var parser = parsers.alloc();
   req.socket = socket;
   req.connection = socket;
-  parser.reinitialize(HTTPParser.RESPONSE);
+  if (destroyHooksExist() && parser.needsAsyncReset && parser.getAsyncId()) {
+    emitDestroy(parser.getAsyncId());
+  }
+  parser.reinitialize(HTTPParser.RESPONSE, parser.needsAsyncReset);
   parser.socket = socket;
   parser.outgoing = req;
   req.parser = parser;

lib/_http_server.js

@@ -40,6 +40,8 @@ const { OutgoingMessage } = require('_http_outgoing');
 const { outHeadersKey, ondrain } = require('internal/http');
 const {
   defaultTriggerAsyncIdScope,
+  destroyHooksExist,
+  emitDestroy,
   getOrSetAsyncId
 } = require('internal/async_hooks');
 const { IncomingMessage } = require('_http_incoming');
@@ -338,7 +340,10 @@ function connectionListenerInternal(server, socket) {
   socket.on('timeout', socketOnTimeout);
 
   var parser = parsers.alloc();
-  parser.reinitialize(HTTPParser.REQUEST);
+  if (destroyHooksExist() && parser.needsAsyncReset && parser.getAsyncId()) {
+    emitDestroy(parser.getAsyncId());
+  }
+  parser.reinitialize(HTTPParser.REQUEST, parser.needsAsyncReset);
   parser.socket = socket;
   socket.parser = parser;
 

lib/internal/freelist.js

@@ -10,8 +10,8 @@ class FreeList {
 
   alloc() {
     return this.list.length ?
-      this.list.pop() :
-      this.ctor.apply(this, arguments);
+      needsToCallAsyncReset(this.list.pop()) :
+      mustNotCallAsyncReset(this.ctor.apply(this, arguments));
   }
 
   free(obj) {
@@ -23,4 +23,14 @@ class FreeList {
   }
 }
 
+function needsToCallAsyncReset(item) {
+  item.needsAsyncReset = true;
+  return item;
+}
+
+function mustNotCallAsyncReset(item) {
+  item.needsAsyncReset = false;
+  return item;
+}
+
 module.exports = FreeList;

src/node_http_parser.cc

@@ -465,6 +465,8 @@ class Parser : public AsyncWrap, public StreamListener {
     Environment* env = Environment::GetCurrent(args);
 
     CHECK(args[0]->IsInt32());
+    CHECK(args[1]->IsBoolean());
+    bool needsAsyncReset = args[1]->IsTrue();
     http_parser_type type =
         static_cast<http_parser_type>(args[0].As<Int32>()->Value());
 
@@ -473,8 +475,12 @@ class Parser : public AsyncWrap, public StreamListener {
     ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
     // Should always be called from the same context.
     CHECK_EQ(env, parser->env());
-    // The parser is being reused. Reset the async id and call init() callbacks.
-    parser->AsyncReset();
+    // 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 (needsAsyncReset) {
+      parser->AsyncReset();
+    }
     parser->Init(type);
   }
 

test/async-hooks/test-graph.http.js

@@ -38,20 +38,14 @@ process.on('exit', function() {
       { type: 'HTTPPARSER',
         id: 'httpparser:1',
         triggerAsyncId: 'tcpserver:1' },
-      { type: 'HTTPPARSER',
-        id: 'httpparser:2',
-        triggerAsyncId: 'tcpserver:1' },
       { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' },
       { type: 'Timeout', id: 'timeout:1', triggerAsyncId: 'tcp:2' },
       { type: 'HTTPPARSER',
-        id: 'httpparser:3',
-        triggerAsyncId: 'tcp:2' },
-      { type: 'HTTPPARSER',
-        id: 'httpparser:4',
+        id: 'httpparser:2',
         triggerAsyncId: 'tcp:2' },
       { type: 'Timeout',
         id: 'timeout:2',
-        triggerAsyncId: 'httpparser:4' },
+        triggerAsyncId: 'httpparser:2' },
       { type: 'SHUTDOWNWRAP',
         id: 'shutdown:1',
         triggerAsyncId: 'tcp:2' } ]

test/parallel/test-async-hooks-http-parser-destroy.js

@@ -0,0 +1,61 @@
+'use strict';
+const common = require('../common');
+const Countdown = require('../common/countdown');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+const http = require('http');
+
+// Regression test for https://github.com/nodejs/node/issues/19859.
+// Checks that matching destroys are emitted when creating new/reusing old http
+// parser instances.
+
+const N = 50;
+const KEEP_ALIVE = 100;
+
+const createdIds = [];
+const destroyedIds = [];
+async_hooks.createHook({
+  init: common.mustCallAtLeast((asyncId, type) => {
+    if (type === 'HTTPPARSER') {
+      createdIds.push(asyncId);
+    }
+  }, N),
+  destroy: (asyncId) => {
+    destroyedIds.push(asyncId);
+  }
+}).enable();
+
+const server = http.createServer(function(req, res) {
+  res.end('Hello');
+});
+
+const keepAliveAgent = new http.Agent({
+  keepAlive: true,
+  keepAliveMsecs: KEEP_ALIVE,
+});
+
+const countdown = new Countdown(N, () => {
+  server.close(() => {
+    // give the server sockets time to close (which will also free their
+    // associated parser objects) after the server has been closed.
+    setTimeout(() => {
+      createdIds.forEach((createdAsyncId) => {
+        assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0);
+      });
+    }, KEEP_ALIVE * 2);
+  });
+});
+
+server.listen(0, function() {
+  for (let i = 0; i < N; ++i) {
+    (function makeRequest() {
+      http.get({
+        port: server.address().port,
+        agent: keepAliveAgent
+      }, function(res) {
+        countdown.dec();
+        res.resume();
+      });
+    })();
+  }
+});

test/parallel/test-freelist.js

@@ -8,24 +8,23 @@ const FreeList = require('internal/freelist');
 
 assert.strictEqual(typeof FreeList, 'function');
 
-const flist1 = new FreeList('flist1', 3, String);
+const flist1 = new FreeList('flist1', 3, Object);
 
 // Allocating when empty, should not change the list size
-const result = flist1.alloc('test');
-assert.strictEqual(typeof result, 'string');
-assert.strictEqual(result, 'test');
+const result = flist1.alloc();
+assert.strictEqual(typeof result, 'object');
 assert.strictEqual(flist1.list.length, 0);
 
 // Exhaust the free list
-assert(flist1.free('test1'));
-assert(flist1.free('test2'));
-assert(flist1.free('test3'));
+assert(flist1.free({ id: 'test1' }));
+assert(flist1.free({ id: 'test2' }));
+assert(flist1.free({ id: 'test3' }));
 
 // Now it should not return 'true', as max length is exceeded
-assert.strictEqual(flist1.free('test4'), false);
-assert.strictEqual(flist1.free('test5'), false);
+assert.strictEqual(flist1.free({ id: 'test4' }), false);
+assert.strictEqual(flist1.free({ id: 'test5' }), false);
 
 // At this point 'alloc' should just return the stored values
-assert.strictEqual(flist1.alloc(), 'test3');
-assert.strictEqual(flist1.alloc(), 'test2');
-assert.strictEqual(flist1.alloc(), 'test1');
+assert.strictEqual(flist1.alloc().id, 'test3');
+assert.strictEqual(flist1.alloc().id, 'test2');
+assert.strictEqual(flist1.alloc().id, 'test1');

test/parallel/test-http-parser.js

@@ -98,7 +98,7 @@ function expectBody(expected) {
     throw new Error('hello world');
   };
 
-  parser.reinitialize(HTTPParser.REQUEST);
+  parser.reinitialize(HTTPParser.REQUEST, false);
 
   assert.throws(
     () => { parser.execute(request, 0, request.length); },
@@ -558,7 +558,7 @@ function expectBody(expected) {
   parser[kOnBody] = expectBody('ping');
   parser.execute(req1, 0, req1.length);
 
-  parser.reinitialize(REQUEST);
+  parser.reinitialize(REQUEST, false);
   parser[kOnBody] = expectBody('pong');
   parser[kOnHeadersComplete] = onHeadersComplete2;
   parser.execute(req2, 0, req2.length);

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

@@ -25,7 +25,7 @@ function execAndClose() {
   process.stdout.write('.');
 
   const parser = parsers.pop();
-  parser.reinitialize(HTTPParser.RESPONSE);
+  parser.reinitialize(HTTPParser.RESPONSE, parser.needsAsyncReset);
 
   const socket = net.connect(common.PORT);
   socket.on('error', (e) => {