From 22901d81c905873486de676c8ca9d13f11503878 Mon Sep 17 00:00:00 2001 From: Sebastian Mayr Date: Thu, 9 Nov 2017 22:57:04 +0100 Subject: [PATCH] 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 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- benchmark/async_hooks/gc-tracking.js | 45 +++++++++++++++++++ doc/api/async_hooks.md | 18 +++++--- lib/async_hooks.js | 21 ++++++++- src/async-wrap.cc | 41 +++++++++++++++++ src/async-wrap.h | 3 ++ src/env.h | 1 + .../test-embedder.api.async-resource.js | 2 +- .../test-async-hooks-destroy-on-gc.js | 27 +++++++++++ .../test-async-hooks-disable-gc-tracking.js | 21 +++++++++ ...test-async-hooks-prevent-double-destroy.js | 24 ++++++++++ 10 files changed, 196 insertions(+), 7 deletions(-) create mode 100644 benchmark/async_hooks/gc-tracking.js create mode 100644 test/parallel/test-async-hooks-destroy-on-gc.js create mode 100644 test/parallel/test-async-hooks-disable-gc-tracking.js create mode 100644 test/parallel/test-async-hooks-prevent-double-destroy.js diff --git a/benchmark/async_hooks/gc-tracking.js b/benchmark/async_hooks/gc-tracking.js new file mode 100644 index 00000000000000..c71c1b07aa5431 --- /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 ff002218b5e489..dd04b7b28d79cc 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 64bac55d2c7d7d..3f00a772bef720 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 c53929d11bba60..75af5c1a081938 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -427,6 +427,46 @@ static void DisablePromiseHook(const FunctionCallbackInfo& args) { } +class DestroyParam { + public: + double asyncId; + v8::Persistent target; + v8::Persistent propBag; +}; + + +void AsyncWrap::WeakCallback(const v8::WeakCallbackInfo& info) { + HandleScope scope(info.GetIsolate()); + + Environment* env = Environment::GetCurrent(info.GetIsolate()); + DestroyParam* p = info.GetParameter(); + Local prop_bag = PersistentToLocal(info.GetIsolate(), p->propBag); + + Local 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& 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()->Value(); + p->target.Reset(isolate, args[0].As()); + p->propBag.Reset(isolate, args[2].As()); + p->target.SetWeak( + p, AsyncWrap::WeakCallback, v8::WeakCallbackType::kParameter); +} + + void AsyncWrap::GetAsyncId(const FunctionCallbackInfo& args) { AsyncWrap* wrap; args.GetReturnValue().Set(-1); @@ -502,6 +542,7 @@ void AsyncWrap::Initialize(Local 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::ReadOnly | v8::DontDelete); diff --git a/src/async-wrap.h b/src/async-wrap.h index 39b6f287c93738..222cda1fd7e27a 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 &info); + private: friend class PromiseWrap; diff --git a/src/env.h b/src/env.h index e37dcf17aeb1d6..75bc210cfb2746 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 f4dfba89557871..eeeaa447c9668c 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 00000000000000..fe6325e189734b --- /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 00000000000000..a34739a9bb2b95 --- /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 00000000000000..5cd9c5e9a017cb --- /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(); + }); +});