async_hooks: fixup do not reuse HTTPParser

Fix some issues introduced/not fixed via
https://github.com/nodejs/node/pull/25094:
* Init hook is not emitted for a reused HTTPParser
* HTTPParser was still used as resource in init hook
* type used in init hook was always HTTPINCOMINGMESSAGE even for client
requests
* some tests have not been adapted to new resource names

With this change the async hooks init event is emitted during a call
to Initialize() as the type and resource object is available at this
time. As a result Initialize() must be called now which could be seen
as breaking change even HTTPParser is not part of documented API.

It was needed to put the ClientRequest instance into a wrapper object
instead passing it directly as async resource otherwise
test-domain-multi fails. I think this is because adding an EventEmitter
to a Domain adds a property 'domain' and the presence of this changes
the context propagation in domains.

Besides that tests still refering to resource HTTPParser have been
updated/improved.

Fixes: https://github.com/nodejs/node/issues/27467
Fixes: https://github.com/nodejs/node/issues/26961
Refs: https://github.com/nodejs/node/pull/25094

PR-URL: https://github.com/nodejs/node/pull/27477
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

13 files changed, 86 insertions, 29 deletions

diff --git a/test/async-hooks/coverage.md b/test/async-hooks/coverage.md
index 0a2af0d06b..1ba18a9383 100644
--- a/test/async-hooks/coverage.md
+++ b/test/async-hooks/coverage.md
@@ -9,7 +9,8 @@ Showing which kind of async resource is covered by which test:
 | FSREQCALLBACK        | test-fsreqcallback-{access,readFile}.js    |
 | GETADDRINFOREQWRAP   | test-getaddrinforeqwrap.js                 |
 | GETNAMEINFOREQWRAP   | test-getnameinforeqwrap.js                 |
-| HTTPPARSER           | test-httpparser.{request,response}.js      |
+| HTTPINCOMINGMESSAGE  | test-httpparser.request.js                 |
+| HTTPCLIENTREQUEST    | test-httpparser.response.js                |
 | Immediate            | test-immediate.js                          |
 | JSSTREAM             | TODO (crashes when accessing directly)     |
 | PBKDF2REQUEST        | test-crypto-pbkdf2.js                      |
diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js
index 3461974ef9..55b9b055a0 100644
--- a/test/async-hooks/test-graph.http.js
+++ b/test/async-hooks/test-graph.http.js
@@ -35,13 +35,13 @@ process.on('exit', function() {
       { type: 'TCPCONNECTWRAP',
         id: 'tcpconnect:1',
         triggerAsyncId: 'tcp:1' },
-      { type: 'HTTPPARSER',
-        id: 'httpparser:1',
+      { type: 'HTTPCLIENTREQUEST',
+        id: 'httpclientrequest:1',
         triggerAsyncId: 'tcpserver:1' },
       { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' },
       { type: 'Timeout', id: 'timeout:1', triggerAsyncId: 'tcp:2' },
-      { type: 'HTTPPARSER',
-        id: 'httpparser:2',
+      { type: 'HTTPINCOMINGMESSAGE',
+        id: 'httpincomingmessage:1',
         triggerAsyncId: 'tcp:2' },
       { type: 'Timeout',
         id: 'timeout:2',
diff --git a/test/async-hooks/test-graph.tls-write.js b/test/async-hooks/test-graph.tls-write.js
index 580264316d..5aee38e6b6 100644
--- a/test/async-hooks/test-graph.tls-write.js
+++ b/test/async-hooks/test-graph.tls-write.js
@@ -64,12 +64,8 @@ function onexit() {
         id: 'getaddrinforeq:1', triggerAsyncId: 'tls:1' },
       { type: 'TCPCONNECTWRAP',
         id: 'tcpconnect:1', triggerAsyncId: 'tcp:1' },
-      { type: 'WRITEWRAP', id: 'write:1', triggerAsyncId: 'tcpconnect:1' },
       { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' },
       { type: 'TLSWRAP', id: 'tls:2', triggerAsyncId: 'tcpserver:1' },
-      { type: 'WRITEWRAP', id: 'write:2', triggerAsyncId: null },
-      { type: 'WRITEWRAP', id: 'write:3', triggerAsyncId: null },
-      { type: 'WRITEWRAP', id: 'write:4', triggerAsyncId: null },
       { type: 'Immediate', id: 'immediate:1', triggerAsyncId: 'tcp:2' },
       { type: 'Immediate', id: 'immediate:2', triggerAsyncId: 'tcp:1' },
     ]
diff --git a/test/async-hooks/test-httparser-reuse.js b/test/async-hooks/test-httparser-reuse.js
index b6d82d7d5e..06441562e0 100644
--- a/test/async-hooks/test-httparser-reuse.js
+++ b/test/async-hooks/test-httparser-reuse.js
@@ -1,28 +1,54 @@
 'use strict';
 
 const common = require('../common');
-const http = require('http');
 const assert = require('assert');
 const { createHook } = require('async_hooks');
+const http = require('http');
+
+// Verify that resource emitted for an HTTPParser is not reused.
+// Verify that correct create/destroy events are emitted.
+
 const reused = Symbol('reused');
 
-let reusedHTTPParser = false;
-const asyncHook = createHook({
+const reusedParser = [];
+const incomingMessageParser = [];
+const clientRequestParser = [];
+const dupDestroys = [];
+const destroyed = [];
+
+createHook({
   init(asyncId, type, triggerAsyncId, resource) {
+    switch (type) {
+      case 'HTTPINCOMINGMESSAGE':
+        incomingMessageParser.push(asyncId);
+        break;
+      case 'HTTPCLIENTREQUEST':
+        clientRequestParser.push(asyncId);
+        break;
+    }
+
     if (resource[reused]) {
-      reusedHTTPParser = true;
+      reusedParser.push(
+        `resource reused: ${asyncId}, ${triggerAsyncId}, ${type}`
+      );
     }
     resource[reused] = true;
+  },
+  destroy(asyncId) {
+    if (destroyed.includes(asyncId)) {
+      dupDestroys.push(asyncId);
+    } else {
+      destroyed.push(asyncId);
+    }
   }
-});
-asyncHook.enable();
+}).enable();
 
-const server = http.createServer(function(req, res) {
+const server = http.createServer((req, res) => {
   res.end();
 });
 
 const PORT = 3000;
-const url = 'http://127.0.0.1:' + PORT;
+const url = `http://127.0.0.1:${PORT}`;
 
 server.listen(PORT, common.mustCall(() => {
   http.get(url, common.mustCall(() => {
@@ -30,10 +56,21 @@ server.listen(PORT, common.mustCall(() => {
       server.listen(PORT, common.mustCall(() => {
         http.get(url, common.mustCall(() => {
           server.close(common.mustCall(() => {
-            assert.strictEqual(reusedHTTPParser, false);
+            setTimeout(common.mustCall(verify), 200);
           }));
         }));
       }));
     }));
   }));
 }));
+
+function verify() {
+  assert.strictEqual(reusedParser.length, 0);
+
+  assert.strictEqual(incomingMessageParser.length, 2);
+  assert.strictEqual(clientRequestParser.length, 2);
+
+  assert.strictEqual(dupDestroys.length, 0);
+  incomingMessageParser.forEach((id) => assert.ok(destroyed.includes(id)));
+  clientRequestParser.forEach((id) => assert.ok(destroyed.includes(id)));
+}
diff --git a/test/async-hooks/test-httpparser.request.js b/test/async-hooks/test-httpparser.request.js
index 7ba8feefe3..f4552398d3 100644
--- a/test/async-hooks/test-httpparser.request.js
+++ b/test/async-hooks/test-httpparser.request.js
@@ -20,7 +20,8 @@ const request = Buffer.from(
   'GET /hello HTTP/1.1\r\n\r\n'
 );
 
-const parser = new HTTPParser(REQUEST);
+const parser = new HTTPParser();
+parser.initialize(REQUEST, {});
 const as = hooks.activitiesOfTypes('HTTPINCOMINGMESSAGE');
 const httpparser = as[0];
 
diff --git a/test/async-hooks/test-httpparser.response.js b/test/async-hooks/test-httpparser.response.js
index 85d6f76525..a207a62636 100644
--- a/test/async-hooks/test-httpparser.response.js
+++ b/test/async-hooks/test-httpparser.response.js
@@ -25,7 +25,8 @@ const request = Buffer.from(
   'pong'
 );
 
-const parser = new HTTPParser(RESPONSE);
+const parser = new HTTPParser();
+parser.initialize(RESPONSE, {});
 const as = hooks.activitiesOfTypes('HTTPCLIENTREQUEST');
 const httpparser = as[0];
 
diff --git a/test/async-hooks/verify-graph.js b/test/async-hooks/verify-graph.js
index d1e09f92bc..1b188faa14 100644
--- a/test/async-hooks/verify-graph.js
+++ b/test/async-hooks/verify-graph.js
@@ -98,6 +98,18 @@ module.exports = function verifyGraph(hooks, graph) {
     );
   }
   assert.strictEqual(errors.length, 0);
+
+  // Verify that all expected types are present
+  const expTypes = Object.create(null);
+  for (let i = 0; i < graph.length; i++) {
+    if (expTypes[graph[i].type] == null) expTypes[graph[i].type] = 0;
+    expTypes[graph[i].type]++;
+  }
+
+  for (const type in expTypes) {
+    assert.strictEqual(typeSeen[type], expTypes[type],
+                       `Expecting type '${type}' in graph`);
+  }
 };
 
 //
diff --git a/test/parallel/test-async-hooks-http-parser-destroy.js b/test/parallel/test-async-hooks-http-parser-destroy.js
index d2e1071c28..d69c474c1d 100644
--- a/test/parallel/test-async-hooks-http-parser-destroy.js
+++ b/test/parallel/test-async-hooks-http-parser-destroy.js
@@ -16,7 +16,7 @@ const createdIds = [];
 const destroyedIds = [];
 async_hooks.createHook({
   init: common.mustCallAtLeast((asyncId, type) => {
-    if (type === 'HTTPPARSER') {
+    if (type === 'HTTPINCOMINGMESSAGE' || type === 'HTTPCLIENTREQUEST') {
       createdIds.push(asyncId);
     }
   }, N),
@@ -25,7 +25,7 @@ async_hooks.createHook({
   }
 }).enable();
 
-const server = http.createServer(function(req, res) {
+const server = http.createServer((req, res) => {
   res.end('Hello');
 });
 
@@ -39,6 +39,7 @@ const countdown = new Countdown(N, () => {
     // Give the server sockets time to close (which will also free their
     // associated parser objects) after the server has been closed.
     setTimeout(() => {
+      assert.strictEqual(createdIds.length, 2 * N);
       createdIds.forEach((createdAsyncId) => {
         assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0);
       });
diff --git a/test/parallel/test-http-parser-bad-ref.js b/test/parallel/test-http-parser-bad-ref.js
index 0b132d69a2..2c1bfe6748 100644
--- a/test/parallel/test-http-parser-bad-ref.js
+++ b/test/parallel/test-http-parser-bad-ref.js
@@ -24,7 +24,8 @@ function flushPool() {
 function demoBug(part1, part2) {
   flushPool();
 
-  const parser = new HTTPParser(HTTPParser.REQUEST);
+  const parser = new HTTPParser();
+  parser.initialize(HTTPParser.REQUEST, {});
 
   parser.headers = [];
   parser.url = '';
diff --git a/test/parallel/test-http-parser-lazy-loaded.js b/test/parallel/test-http-parser-lazy-loaded.js
index 6d6b2ddd25..79b6ac37b3 100644
--- a/test/parallel/test-http-parser-lazy-loaded.js
+++ b/test/parallel/test-http-parser-lazy-loaded.js
@@ -7,7 +7,10 @@ const { getOptionValue } = require('internal/options');
 
 // Monkey patch before requiring anything
 class DummyParser {
-  constructor(type) {
+  constructor() {
+    this.test_type = null;
+  }
+  initialize(type) {
     this.test_type = type;
   }
 }
@@ -25,6 +28,7 @@ const { parsers } = require('_http_common');
 
 // Test _http_common was not loaded before monkey patching
 const parser = parsers.alloc();
+parser.initialize(DummyParser.REQUEST, {});
 assert.strictEqual(parser instanceof DummyParser, true);
 assert.strictEqual(parser.test_type, DummyParser.REQUEST);
 
diff --git a/test/parallel/test-http-parser.js b/test/parallel/test-http-parser.js
index 078895d49b..97dc57f755 100644
--- a/test/parallel/test-http-parser.js
+++ b/test/parallel/test-http-parser.js
@@ -38,7 +38,8 @@ const kOnMessageComplete = HTTPParser.kOnMessageComplete | 0;
 
 
 function newParser(type) {
-  const parser = new HTTPParser(type);
+  const parser = new HTTPParser();
+  parser.initialize(type, {});
 
   parser.headers = [];
   parser.url = '';
@@ -95,7 +96,7 @@ function expectBody(expected) {
     throw new Error('hello world');
   };
 
-  parser.initialize(HTTPParser.REQUEST, request);
+  parser.initialize(REQUEST, {});
 
   assert.throws(
     () => { parser.execute(request, 0, request.length); },
diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js
index e3cfd6bef0..9f5c073c9e 100644
--- a/test/sequential/test-async-wrap-getasyncid.js
+++ b/test/sequential/test-async-wrap-getasyncid.js
@@ -152,7 +152,10 @@ if (common.hasCrypto) { // eslint-disable-line node-core/crypto-check
 
 {
   const { HTTPParser } = require('_http_common');
-  testInitialized(new HTTPParser(HTTPParser.REQUEST), 'HTTPParser');
+  const parser = new HTTPParser();
+  testUninitialized(parser, 'HTTPParser');
+  parser.initialize(HTTPParser.REQUEST, {});
+  testInitialized(parser, 'HTTPParser');
 }
 
 
diff --git a/test/sequential/test-http-regr-gh-2928.js b/test/sequential/test-http-regr-gh-2928.js
index 400dc02013..5111e234d1 100644
--- a/test/sequential/test-http-regr-gh-2928.js
+++ b/test/sequential/test-http-regr-gh-2928.js
@@ -7,7 +7,6 @@ const common = require('../common');
 const assert = require('assert');
 const httpCommon = require('_http_common');
 const { HTTPParser } = require('_http_common');
-const { AsyncResource } = require('async_hooks');
 const net = require('net');
 
 const COUNT = httpCommon.parsers.max + 1;
@@ -25,7 +24,7 @@ function execAndClose() {
   process.stdout.write('.');
 
   const parser = parsers.pop();
-  parser.initialize(HTTPParser.RESPONSE, new AsyncResource('ClientRequest'));
+  parser.initialize(HTTPParser.RESPONSE, {});
 
   const socket = net.connect(common.PORT);
   socket.on('error', (e) => {