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>

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');
+  }
+}

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:
 

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;
   }

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);

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;
 

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")                                                  \

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,

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)));
+});

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();
+  });
+});

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();
+  });
+});