async_hooks: add destroy event for gced AsyncResources

In cases where libraries create AsyncResources which may be emitting
more events depending on usage, the only way to ensure that destroy is
called properly is by calling it when the resource gets garbage
collected.

Fixes: https://github.com/nodejs/node/issues/16153
PR-URL: https://github.com/nodejs/node/pull/16998
Fixes: https://github.com/nodejs/node/issues/16153
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

10 files changed, 196 insertions, 7 deletions

diff --git a/benchmark/async_hooks/gc-tracking.js b/benchmark/async_hooks/gc-tracking.js
new file mode 100644
index 0000000000..c71c1b07aa
--- /dev/null
+++ b/benchmark/async_hooks/gc-tracking.js
@@ -0,0 +1,45 @@
+'use strict';
+const common = require('../common.js');
+const { AsyncResource } = require('async_hooks');
+
+const bench = common.createBenchmark(main, {
+  n: [1e6],
+  method: [
+    'trackingEnabled',
+    'trackingDisabled',
+  ]
+}, {
+  flags: ['--expose-gc']
+});
+
+function endAfterGC(n) {
+  setImmediate(() => {
+    global.gc();
+    setImmediate(() => {
+      bench.end(n);
+    });
+  });
+}
+
+function main(conf) {
+  const n = +conf.n;
+
+  switch (conf.method) {
+    case 'trackingEnabled':
+      bench.start();
+      for (let i = 0; i < n; i++) {
+        new AsyncResource('foobar');
+      }
+      endAfterGC(n);
+      break;
+    case 'trackingDisabled':
+      bench.start();
+      for (let i = 0; i < n; i++) {
+        new AsyncResource('foobar', { requireManualDestroy: true });
+      }
+      endAfterGC(n);
+      break;
+    default:
+      throw new Error('Unsupported method');
+  }
+}
diff --git a/doc/api/async_hooks.md b/doc/api/async_hooks.md
index ff002218b5..dd04b7b28d 100644
--- a/doc/api/async_hooks.md
+++ b/doc/api/async_hooks.md
@@ -543,12 +543,14 @@ will occur and the process will abort.
 The following is an overview of the `AsyncResource` API.
 
 ```js
-const { AsyncResource } = require('async_hooks');
+const { AsyncResource, executionAsyncId } = require('async_hooks');
 
 // AsyncResource() is meant to be extended. Instantiating a
 // new AsyncResource() also triggers init. If triggerAsyncId is omitted then
 // async_hook.executionAsyncId() is used.
-const asyncResource = new AsyncResource(type, triggerAsyncId);
+const asyncResource = new AsyncResource(
+  type, { triggerAsyncId: executionAsyncId(), requireManualDestroy: false }
+);
 
 // Call AsyncHooks before callbacks.
 asyncResource.emitBefore();
@@ -566,11 +568,17 @@ asyncResource.asyncId();
 asyncResource.triggerAsyncId();
 ```
 
-#### `AsyncResource(type[, triggerAsyncId])`
+#### `AsyncResource(type[, options])`
 
 * `type` {string} The type of async event.
-* `triggerAsyncId` {number} The ID of the execution context that created this
-  async event.
+* `options` {Object}
+  * `triggerAsyncId` {number} The ID of the execution context that created this
+  async event.  **Default:** `executionAsyncId()`
+  * `requireManualDestroy` {boolean} Disables automatic `emitDestroy` when the
+  object is garbage collected. This usually does not need to be set (even if
+  `emitDestroy` is called manually), unless the resource's asyncId is retrieved
+  and the sensitive API's `emitDestroy` is called with it.
+  **Default:** `false`
 
 Example usage:
 
diff --git a/lib/async_hooks.js b/lib/async_hooks.js
index 64bac55d2c..3f00a772be 100644
--- a/lib/async_hooks.js
+++ b/lib/async_hooks.js
@@ -29,6 +29,9 @@ const { async_hook_fields, async_id_fields } = async_wrap;
 const { pushAsyncIds, popAsyncIds } = async_wrap;
 // For performance reasons, only track Proimses when a hook is enabled.
 const { enablePromiseHook, disablePromiseHook } = async_wrap;
+// For userland AsyncResources, make sure to emit a destroy event when the
+// resource gets gced.
+const { registerDestroyHook } = async_wrap;
 // Properties in active_hooks are used to keep track of the set of hooks being
 // executed in case another hook is enabled/disabled. The new set of hooks is
 // then restored once the active set of hooks is finished executing.
@@ -259,13 +262,22 @@ function validateAsyncId(asyncId, type) {
 
 // Embedder API //
 
+const destroyedSymbol = Symbol('destroyed');
+
 class AsyncResource {
-  constructor(type, triggerAsyncId = initTriggerId()) {
+  constructor(type, opts = {}) {
     if (typeof type !== 'string')
       throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'type', 'string');
 
+    if (typeof opts === 'number') {
+      opts = { triggerAsyncId: opts, requireManualDestroy: false };
+    } else if (opts.triggerAsyncId === undefined) {
+      opts.triggerAsyncId = initTriggerId();
+    }
+
     // Unlike emitInitScript, AsyncResource doesn't supports null as the
     // triggerAsyncId.
+    const triggerAsyncId = opts.triggerAsyncId;
     if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) {
       throw new errors.RangeError('ERR_INVALID_ASYNC_ID',
                                   'triggerAsyncId',
@@ -274,10 +286,16 @@ class AsyncResource {
 
     this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter];
     this[trigger_async_id_symbol] = triggerAsyncId;
+    // this prop name (destroyed) has to be synchronized with C++
+    this[destroyedSymbol] = { destroyed: false };
 
     emitInitScript(
       this[async_id_symbol], type, this[trigger_async_id_symbol], this
     );
+
+    if (!opts.requireManualDestroy) {
+      registerDestroyHook(this, this[async_id_symbol], this[destroyedSymbol]);
+    }
   }
 
   emitBefore() {
@@ -291,6 +309,7 @@ class AsyncResource {
   }
 
   emitDestroy() {
+    this[destroyedSymbol].destroyed = true;
     emitDestroyScript(this[async_id_symbol]);
     return this;
   }
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index c53929d11b..75af5c1a08 100644
--- a/src/async-wrap.cc
+++ b/src/async-wrap.cc
@@ -427,6 +427,46 @@ static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
 }
 
 
+class DestroyParam {
+ public:
+  double asyncId;
+  v8::Persistent<Object> target;
+  v8::Persistent<Object> propBag;
+};
+
+
+void AsyncWrap::WeakCallback(const v8::WeakCallbackInfo<DestroyParam>& info) {
+  HandleScope scope(info.GetIsolate());
+
+  Environment* env = Environment::GetCurrent(info.GetIsolate());
+  DestroyParam* p = info.GetParameter();
+  Local<Object> prop_bag = PersistentToLocal(info.GetIsolate(), p->propBag);
+
+  Local<Value> val = prop_bag->Get(env->destroyed_string());
+  if (val->IsFalse()) {
+    AsyncWrap::EmitDestroy(env, p->asyncId);
+  }
+  p->target.Reset();
+  p->propBag.Reset();
+  delete p;
+}
+
+
+static void RegisterDestroyHook(const FunctionCallbackInfo<Value>& args) {
+  CHECK(args[0]->IsObject());
+  CHECK(args[1]->IsNumber());
+  CHECK(args[2]->IsObject());
+
+  Isolate* isolate = args.GetIsolate();
+  DestroyParam* p = new DestroyParam();
+  p->asyncId = args[1].As<Number>()->Value();
+  p->target.Reset(isolate, args[0].As<Object>());
+  p->propBag.Reset(isolate, args[2].As<Object>());
+  p->target.SetWeak(
+    p, AsyncWrap::WeakCallback, v8::WeakCallbackType::kParameter);
+}
+
+
 void AsyncWrap::GetAsyncId(const FunctionCallbackInfo<Value>& args) {
   AsyncWrap* wrap;
   args.GetReturnValue().Set(-1);
@@ -502,6 +542,7 @@ void AsyncWrap::Initialize(Local<Object> target,
   env->SetMethod(target, "queueDestroyAsyncId", QueueDestroyAsyncId);
   env->SetMethod(target, "enablePromiseHook", EnablePromiseHook);
   env->SetMethod(target, "disablePromiseHook", DisablePromiseHook);
+  env->SetMethod(target, "registerDestroyHook", RegisterDestroyHook);
 
   v8::PropertyAttribute ReadOnlyDontDelete =
       static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete);
diff --git a/src/async-wrap.h b/src/async-wrap.h
index 39b6f287c9..222cda1fd7 100644
--- a/src/async-wrap.h
+++ b/src/async-wrap.h
@@ -84,6 +84,7 @@ namespace node {
   NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V)
 
 class Environment;
+class DestroyParam;
 
 class AsyncWrap : public BaseObject {
  public:
@@ -163,6 +164,8 @@ class AsyncWrap : public BaseObject {
 
   virtual size_t self_size() const = 0;
 
+  static void WeakCallback(const v8::WeakCallbackInfo<DestroyParam> &info);
+
  private:
   friend class PromiseWrap;
 
diff --git a/src/env.h b/src/env.h
index e37dcf17ae..75bc210cfb 100644
--- a/src/env.h
+++ b/src/env.h
@@ -120,6 +120,7 @@ class ModuleWrap;
   V(cwd_string, "cwd")                                                        \
   V(dest_string, "dest")                                                      \
   V(destroy_string, "destroy")                                                \
+  V(destroyed_string, "destroyed")                                            \
   V(detached_string, "detached")                                              \
   V(dns_a_string, "A")                                                        \
   V(dns_aaaa_string, "AAAA")                                                  \
diff --git a/test/async-hooks/test-embedder.api.async-resource.js b/test/async-hooks/test-embedder.api.async-resource.js
index f4dfba8955..eeeaa447c9 100644
--- a/test/async-hooks/test-embedder.api.async-resource.js
+++ b/test/async-hooks/test-embedder.api.async-resource.js
@@ -18,7 +18,7 @@ common.expectsError(
     type: TypeError,
   });
 assert.throws(() => {
-  new AsyncResource('invalid_trigger_id', null);
+  new AsyncResource('invalid_trigger_id', { triggerAsyncId: null });
 }, common.expectsError({
   code: 'ERR_INVALID_ASYNC_ID',
   type: RangeError,
diff --git a/test/parallel/test-async-hooks-destroy-on-gc.js b/test/parallel/test-async-hooks-destroy-on-gc.js
new file mode 100644
index 0000000000..fe6325e189
--- /dev/null
+++ b/test/parallel/test-async-hooks-destroy-on-gc.js
@@ -0,0 +1,27 @@
+'use strict';
+// Flags: --expose_gc
+
+// This test ensures that userland-only AsyncResources cause a destroy event to
+// be emitted when they get gced.
+
+const common = require('../common');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+
+const destroyedIds = new Set();
+async_hooks.createHook({
+  destroy: common.mustCallAtLeast((asyncId) => {
+    destroyedIds.add(asyncId);
+  }, 1)
+}).enable();
+
+let asyncId = null;
+{
+  const res = new async_hooks.AsyncResource('foobar');
+  asyncId = res.asyncId();
+}
+
+setImmediate(() => {
+  global.gc();
+  setImmediate(() => assert.ok(destroyedIds.has(asyncId)));
+});
diff --git a/test/parallel/test-async-hooks-disable-gc-tracking.js b/test/parallel/test-async-hooks-disable-gc-tracking.js
new file mode 100644
index 0000000000..a34739a9bb
--- /dev/null
+++ b/test/parallel/test-async-hooks-disable-gc-tracking.js
@@ -0,0 +1,21 @@
+'use strict';
+// Flags: --expose_gc
+
+// This test ensures that userland-only AsyncResources cause a destroy event to
+// be emitted when they get gced.
+
+const common = require('../common');
+const async_hooks = require('async_hooks');
+
+const hook = async_hooks.createHook({
+  destroy: common.mustCall(1) // only 1 immediate is destroyed
+}).enable();
+
+new async_hooks.AsyncResource('foobar', { requireManualDestroy: true });
+
+setImmediate(() => {
+  global.gc();
+  setImmediate(() => {
+    hook.disable();
+  });
+});
diff --git a/test/parallel/test-async-hooks-prevent-double-destroy.js b/test/parallel/test-async-hooks-prevent-double-destroy.js
new file mode 100644
index 0000000000..5cd9c5e9a0
--- /dev/null
+++ b/test/parallel/test-async-hooks-prevent-double-destroy.js
@@ -0,0 +1,24 @@
+'use strict';
+// Flags: --expose_gc
+
+// This test ensures that userland-only AsyncResources cause a destroy event to
+// be emitted when they get gced.
+
+const common = require('../common');
+const async_hooks = require('async_hooks');
+
+const hook = async_hooks.createHook({
+  destroy: common.mustCall(2) // 1 immediate + manual destroy
+}).enable();
+
+{
+  const res = new async_hooks.AsyncResource('foobar');
+  res.emitDestroy();
+}
+
+setImmediate(() => {
+  global.gc();
+  setImmediate(() => {
+    hook.disable();
+  });
+});