async_hooks: avoid double-destroy HTTPParser

Avoid that destroy hook is invoked twice - once via `Parser::Free()`
and once again via `Parser::Reinitialize()` by clearing the async_id
in `EmitDestroy()`.

Partial backport of https://github.com/nodejs/node/pull/27477, a full
backport would require also https://github.com/nodejs/node/pull/25094
which has a dont-land-on-v10.x label on it.

Fixes: https://github.com/nodejs/node/issues/26961

Backport-PR-URL: https://github.com/nodejs/node/pull/27986
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>

3 files changed, 27 insertions, 18 deletions

diff --git a/src/async_wrap.cc b/src/async_wrap.cc
index 86142d13a32..6eb06cc81e9 100644
--- a/src/async_wrap.cc
+++ b/src/async_wrap.cc
@@ -176,7 +176,7 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
 class PromiseWrap : public AsyncWrap {
  public:
   PromiseWrap(Environment* env, Local<Object> object, bool silent)
-      : AsyncWrap(env, object, PROVIDER_PROMISE, -1, silent) {
+      : AsyncWrap(env, object, PROVIDER_PROMISE, kInvalidAsyncId, silent) {
     MakeWeak();
   }
 
@@ -382,7 +382,7 @@ static void RegisterDestroyHook(const FunctionCallbackInfo<Value>& args) {
 
 void AsyncWrap::GetAsyncId(const FunctionCallbackInfo<Value>& args) {
   AsyncWrap* wrap;
-  args.GetReturnValue().Set(-1);
+  args.GetReturnValue().Set(kInvalidAsyncId);
   ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
   args.GetReturnValue().Set(wrap->get_async_id());
 }
@@ -409,10 +409,15 @@ void AsyncWrap::AsyncReset(const FunctionCallbackInfo<Value>& args) {
   AsyncWrap* wrap;
   ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
   double execution_async_id =
-      args[0]->IsNumber() ? args[0].As<Number>()->Value() : -1;
+      args[0]->IsNumber() ? args[0].As<Number>()->Value() : kInvalidAsyncId;
   wrap->AsyncReset(execution_async_id);
 }
 
+void AsyncWrap::EmitDestroy() {
+  AsyncWrap::EmitDestroy(env(), async_id_);
+  // Ensure no double destroy is emitted via AsyncReset().
+  async_id_ = kInvalidAsyncId;
+}
 
 void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[0]->IsNumber());
@@ -474,7 +479,7 @@ void AsyncWrap::Initialize(Local<Object> target,
   // kDefaultTriggerAsyncId: Write the id of the resource responsible for a
   //   handle's creation just before calling the new handle's constructor.
   //   After the new handle is constructed kDefaultTriggerAsyncId is set back
-  //   to -1.
+  //   to kInvalidAsyncId.
   FORCE_SET_TARGET_FIELD(target,
                          "async_id_fields",
                          env->async_hooks()->async_id_fields().GetJSArray());
@@ -558,7 +563,7 @@ AsyncWrap::AsyncWrap(Environment* env,
   CHECK_NE(provider, PROVIDER_NONE);
   CHECK_GE(object->InternalFieldCount(), 1);
 
-  async_id_ = -1;
+  async_id_ = kInvalidAsyncId;
   // Use AsyncReset() call to execute the init() callbacks.
   AsyncReset(execution_async_id, silent);
 }
@@ -566,7 +571,7 @@ AsyncWrap::AsyncWrap(Environment* env,
 
 AsyncWrap::~AsyncWrap() {
   EmitTraceEventDestroy();
-  EmitDestroy(env(), get_async_id());
+  EmitDestroy();
 }
 
 void AsyncWrap::EmitTraceEventDestroy() {
@@ -602,16 +607,16 @@ 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) {
+  if (async_id_ != kInvalidAsyncId) {
     // 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_);
+    EmitDestroy();
   }
 
   // Now we can assign a new async_id_ to this instance.
-  async_id_ =
-    execution_async_id == -1 ? env()->new_async_id() : execution_async_id;
+  async_id_ = execution_async_id == kInvalidAsyncId ? env()->new_async_id()
+                                                     : execution_async_id;
   trigger_async_id_ = env()->get_default_trigger_async_id();
 
   switch (provider_type()) {
@@ -693,7 +698,7 @@ async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
   // Environment::GetCurrent() allocates a Local<> handle.
   HandleScope handle_scope(isolate);
   Environment* env = Environment::GetCurrent(isolate);
-  if (env == nullptr) return -1;
+  if (env == nullptr) return AsyncWrap::kInvalidAsyncId;
   return env->execution_async_id();
 }
 
@@ -702,7 +707,7 @@ async_id AsyncHooksGetTriggerAsyncId(Isolate* isolate) {
   // Environment::GetCurrent() allocates a Local<> handle.
   HandleScope handle_scope(isolate);
   Environment* env = Environment::GetCurrent(isolate);
-  if (env == nullptr) return -1;
+  if (env == nullptr) return AsyncWrap::kInvalidAsyncId;
   return env->trigger_async_id();
 }
 
@@ -727,7 +732,7 @@ async_context EmitAsyncInit(Isolate* isolate,
   CHECK_NOT_NULL(env);
 
   // Initialize async context struct
-  if (trigger_async_id == -1)
+  if (trigger_async_id == AsyncWrap::kInvalidAsyncId)
     trigger_async_id = env->get_default_trigger_async_id();
 
   async_context context = {
diff --git a/src/async_wrap.h b/src/async_wrap.h
index c342898b143..3e22fa20b68 100644
--- a/src/async_wrap.h
+++ b/src/async_wrap.h
@@ -108,10 +108,12 @@ class AsyncWrap : public BaseObject {
   AsyncWrap(Environment* env,
             v8::Local<v8::Object> object,
             ProviderType provider,
-            double execution_async_id = -1);
+            double execution_async_id = kInvalidAsyncId);
 
   virtual ~AsyncWrap();
 
+  static constexpr double kInvalidAsyncId = -1;
+
   static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
       Environment* env);
 
@@ -137,6 +139,8 @@ class AsyncWrap : public BaseObject {
   static void EmitAfter(Environment* env, double async_id);
   static void EmitPromiseResolve(Environment* env, double async_id);
 
+  void EmitDestroy();
+
   void EmitTraceEventBefore();
   static void EmitTraceEventAfter(ProviderType type, double async_id);
   void EmitTraceEventDestroy();
@@ -149,7 +153,8 @@ class AsyncWrap : public BaseObject {
 
   inline double get_trigger_async_id() const;
 
-  void AsyncReset(double execution_async_id = -1, bool silent = false);
+  void AsyncReset(double execution_async_id = kInvalidAsyncId,
+                  bool silent = false);
 
   // Only call these within a valid HandleScope.
   v8::MaybeLocal<v8::Value> MakeCallback(const v8::Local<v8::Function> cb,
@@ -202,7 +207,7 @@ class AsyncWrap : public BaseObject {
   inline AsyncWrap();
   const ProviderType provider_type_;
   // Because the values may be Reset(), cannot be made const.
-  double async_id_ = -1;
+  double async_id_ = kInvalidAsyncId;
   double trigger_async_id_;
 };
 
diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
index b29cdf4b28b..c2cd7a213bb 100644
--- a/src/node_http_parser.cc
+++ b/src/node_http_parser.cc
@@ -383,14 +383,13 @@ class Parser : public AsyncWrap, public StreamListener {
 
 
   static void Free(const FunctionCallbackInfo<Value>& args) {
-    Environment* env = Environment::GetCurrent(args);
     Parser* parser;
     ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
 
     // Since the Parser destructor isn't going to run the destroy() callbacks
     // it needs to be triggered manually.
     parser->EmitTraceEventDestroy();
-    parser->EmitDestroy(env, parser->get_async_id());
+    parser->EmitDestroy();
   }