Skip to content

Commit

Permalink
async_hooks: add destroy event for gced AsyncResources
Browse files Browse the repository at this point in the history
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: #16153
PR-URL: #16998
Fixes: #16153
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
Sebmaster authored and addaleax committed Nov 16, 2017
1 parent b3127cd commit 22901d8
Show file tree
Hide file tree
Showing 10 changed files with 196 additions and 7 deletions.
45 changes: 45 additions & 0 deletions benchmark/async_hooks/gc-tracking.js
Original file line number Diff line number Diff line change
@@ -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');
}
}
18 changes: 13 additions & 5 deletions doc/api/async_hooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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:

Expand Down
21 changes: 20 additions & 1 deletion lib/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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',
Expand All @@ -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() {
Expand All @@ -291,6 +309,7 @@ class AsyncResource {
}

emitDestroy() {
this[destroyedSymbol].destroyed = true;
emitDestroyScript(this[async_id_symbol]);
return this;
}
Expand Down
41 changes: 41 additions & 0 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions src/async-wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ namespace node {
NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V)

class Environment;
class DestroyParam;

class AsyncWrap : public BaseObject {
public:
Expand Down Expand Up @@ -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;

Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand Down
2 changes: 1 addition & 1 deletion test/async-hooks/test-embedder.api.async-resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-async-hooks-destroy-on-gc.js
Original file line number Diff line number Diff line change
@@ -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)));
});
21 changes: 21 additions & 0 deletions test/parallel/test-async-hooks-disable-gc-tracking.js
Original file line number Diff line number Diff line change
@@ -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();
});
});
24 changes: 24 additions & 0 deletions test/parallel/test-async-hooks-prevent-double-destroy.js
Original file line number Diff line number Diff line change
@@ -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();
});
});

0 comments on commit 22901d8

Please sign in to comment.