async_hooks: add missing async_hooks destroys in AsyncReset

This adds missing async_hooks destroy calls for sockets (in
_http_agent.js) and HTTP parsers. We need to emit a destroy in
AsyncWrap#AsyncReset before assigning a new async_id when the instance
has already been in use and is being recycled, because in that case, we
have already emitted an init for the "old" async_id.

This also removes a duplicated init call 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.

PR-URL: https://github.com/nodejs/node/pull/23272
Fixes: https://github.com/nodejs/node/issues/19859
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

2 files changed, 17 insertions, 2 deletions

diff --git a/src/async_wrap.cc b/src/async_wrap.cc
index 2b163a5fa28..596fcc8356d 100644
--- a/src/async_wrap.cc
+++ b/src/async_wrap.cc
@@ -563,6 +563,7 @@ AsyncWrap::AsyncWrap(Environment* env,
   CHECK_NE(provider, PROVIDER_NONE);
   CHECK_GE(object->InternalFieldCount(), 1);
 
+  async_id_ = -1;
   // Use AsyncReset() call to execute the init() callbacks.
   AsyncReset(execution_async_id, silent);
 }
@@ -606,6 +607,14 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
 // and reused over their lifetime. This way a new uid can be assigned when
 // the resource is pulled out of the pool and put back into use.
 void AsyncWrap::AsyncReset(double execution_async_id, bool silent) {
+  if (async_id_ != -1) {
+    // This instance was in use before, we have already emitted an init with
+    // its previous async_id and need to emit a matching destroy for that
+    // before generating a new async_id.
+    EmitDestroy(env(), async_id_);
+  }
+
+  // Now we can assign a new async_id_ to this instance.
   async_id_ =
     execution_async_id == -1 ? env()->new_async_id() : execution_async_id;
   trigger_async_id_ = env()->get_default_trigger_async_id();
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
index 5d093b27c39..9850b4f6982 100644
--- a/src/node_http_parser.cc
+++ b/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);
   }