http: remove duplicate async_hooks init for http parser

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);
   }

benchmark/misc/freelist.js

@@ -9,7 +9,7 @@ const bench = common.createBenchmark(main, {
 });
 
 function main({ n }) {
-  const FreeList = require('internal/freelist');
+  const { FreeList } = require('internal/freelist');
   const poolSize = 1000;
   const list = new FreeList('test', poolSize, Object);
   var j;

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 {
@@ -47,6 +51,7 @@ 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]/;
 
@@ -631,7 +636,10 @@ function tickOnSocket(req, socket) {
   var parser = parsers.alloc();
   req.socket = socket;
   req.connection = socket;
-  parser.reinitialize(HTTPParser.RESPONSE);
+  if (destroyHooksExist() && parser[is_reused_symbol] && parser.getAsyncId()) {
+    emitDestroy(parser.getAsyncId());
+  }
+  parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]);
   parser.socket = socket;
   parser.outgoing = req;
   req.parser = parser;

lib/_http_common.js

@@ -23,7 +23,7 @@
 
 const { methods, HTTPParser } = internalBinding('http_parser');
 
-const FreeList = require('internal/freelist');
+const { FreeList } = require('internal/freelist');
 const { ondrain } = require('internal/http');
 const incoming = require('_http_incoming');
 const {

lib/_http_server.js

@@ -40,8 +40,11 @@ const { OutgoingMessage } = require('_http_outgoing');
 const { outHeadersKey, ondrain } = require('internal/http');
 const {
   defaultTriggerAsyncIdScope,
+  destroyHooksExist,
+  emitDestroy,
   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,7 +341,10 @@ function connectionListenerInternal(server, socket) {
   socket.on('timeout', socketOnTimeout);
 
   var parser = parsers.alloc();
-  parser.reinitialize(HTTPParser.REQUEST);
+  if (destroyHooksExist() && parser[is_reused_symbol] && parser.getAsyncId()) {
+    emitDestroy(parser.getAsyncId());
+  }
+  parser.reinitialize(HTTPParser.REQUEST, parser[is_reused_symbol]);
   parser.socket = socket;
   socket.parser = parser;
 

lib/internal/freelist.js

@@ -1,5 +1,7 @@
 'use strict';
 
+const is_reused_symbol = Symbol('isReused');
+
 class FreeList {
   constructor(name, max, ctor) {
     this.name = name;
@@ -10,8 +12,8 @@ class FreeList {
 
   alloc() {
     return this.list.length ?
-      this.list.pop() :
-      this.ctor.apply(this, arguments);
+      setIsReused(this.list.pop(), true) :
+      setIsReused(this.ctor.apply(this, arguments), false);
   }
 
   free(obj) {
@@ -23,4 +25,14 @@ class FreeList {
   }
 }
 
-module.exports = FreeList;
+function setIsReused(item, reused) {
+  item[is_reused_symbol] = reused;
+  return item;
+}
+
+module.exports = {
+  FreeList,
+  symbols: {
+    is_reused_symbol
+  }
+};

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 isReused = 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 (isReused) {
+      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

@@ -4,28 +4,27 @@
 
 require('../common');
 const assert = require('assert');
-const FreeList = require('internal/freelist');
+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/parallel/test-internal-modules-expose.js

@@ -7,5 +7,5 @@ const config = process.binding('config');
 
 console.log(config, process.argv);
 
-assert.strictEqual(typeof require('internal/freelist'), 'function');
+assert.strictEqual(typeof require('internal/freelist').FreeList, 'function');
 assert.strictEqual(config.exposeInternals, true);

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

@@ -7,6 +7,7 @@ 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');
 
@@ -25,7 +26,7 @@ function execAndClose() {
   process.stdout.write('.');
 
   const parser = parsers.pop();
-  parser.reinitialize(HTTPParser.RESPONSE);
+  parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]);
 
   const socket = net.connect(common.PORT);
   socket.on('error', (e) => {