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