From 82ca3545514e7f31a92b890583ace62e5bca1081 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Mon, 10 Jul 2017 14:15:10 +0200 Subject: [PATCH 1/4] async_hooks: rename internal emit functions There are two categories of emit functions in async_hooks, those used by c++ (native) and those used by JavaScript (script). Previously these were named N for native and S for script. Finally, there was an odd case where emitInitN was called just init. This makes it more explicit by using the names emitInitNative and emitInitScript. The other emit functions are also renamed. --- lib/async_hooks.js | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 4056fe1635f7c8..7758c8d30577a0 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -57,7 +57,7 @@ const emitDestroyNative = emitHookFactory(destroy_symbol, 'emitDestroyNative'); // process. They use the same functions as the JS embedder API. These callbacks // are setup immediately to prevent async_wrap.setupHooks() from being hijacked // and the cost of doing so is negligible. -async_wrap.setupHooks({ init, +async_wrap.setupHooks({ init: emitInitNative, before: emitBeforeNative, after: emitAfterNative, destroy: emitDestroyNative }); @@ -228,21 +228,21 @@ class AsyncResource { if (async_hook_fields[kInit] === 0) return; - init(this[async_id_symbol], type, triggerAsyncId, this); + emitInitNative(this[async_id_symbol], type, triggerAsyncId, this); } emitBefore() { - emitBeforeS(this[async_id_symbol], this[trigger_id_symbol]); + emitBeforeScript(this[async_id_symbol], this[trigger_id_symbol]); return this; } emitAfter() { - emitAfterS(this[async_id_symbol]); + emitAfterScript(this[async_id_symbol]); return this; } emitDestroy() { - emitDestroyS(this[async_id_symbol]); + emitDestroyScript(this[async_id_symbol]); return this; } @@ -311,7 +311,7 @@ function setInitTriggerId(triggerAsyncId) { } -function emitInitS(asyncId, type, triggerAsyncId, resource) { +function emitInitScript(asyncId, type, triggerAsyncId, resource) { // Short circuit all checks for the common case. Which is that no hooks have // been set. Do this to remove performance impact for embedders (and core). // Even though it bypasses all the argument checks. The performance savings @@ -334,7 +334,7 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) { if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0) throw new RangeError('triggerAsyncId must be an unsigned integer'); - init(asyncId, type, triggerAsyncId, resource); + emitInitNative(asyncId, type, triggerAsyncId, resource); } function emitHookFactory(symbol, name) { @@ -370,9 +370,7 @@ function emitHookFactory(symbol, name) { } -// Usage: emitBeforeS(asyncId[, triggerAsyncId]). If triggerAsyncId is omitted -// then asyncId will be used instead. -function emitBeforeS(asyncId, triggerAsyncId) { +function emitBeforeScript(asyncId, triggerAsyncId) { // CHECK(Number.isSafeInteger(asyncId) && asyncId > 0) // CHECK(Number.isSafeInteger(triggerAsyncId) && triggerAsyncId > 0) @@ -392,7 +390,7 @@ function emitBeforeS(asyncId, triggerAsyncId) { // TODO(trevnorris): Calling emitBefore/emitAfter from native can't adjust the // kIdStackIndex. But what happens if the user doesn't have both before and // after callbacks. -function emitAfterS(asyncId) { +function emitAfterScript(asyncId) { if (async_hook_fields[kAfter] > 0) emitAfterNative(asyncId); @@ -400,7 +398,7 @@ function emitAfterS(asyncId) { } -function emitDestroyS(asyncId) { +function emitDestroyScript(asyncId) { // Return early if there are no destroy callbacks, or on attempt to emit // destroy on the void. if (async_hook_fields[kDestroy] === 0 || asyncId === 0) @@ -422,7 +420,7 @@ function emitDestroyS(asyncId) { // change in the future depending on whether it can be determined if there's a // slim chance of the application remaining stable after handling one of these // exceptions. -function init(asyncId, type, triggerAsyncId, resource) { +function emitInitNative(asyncId, type, triggerAsyncId, resource) { processing_hook += 1; // Use a single try/catch for all hook to avoid setting up one per iteration. try { @@ -467,10 +465,10 @@ module.exports = { newUid, initTriggerId, setInitTriggerId, - emitInit: emitInitS, - emitBefore: emitBeforeS, - emitAfter: emitAfterS, - emitDestroy: emitDestroyS, + emitInit: emitInitScript, + emitBefore: emitBeforeScript, + emitAfter: emitAfterScript, + emitDestroy: emitDestroyScript, }; // currentId was renamed to executionAsyncId. This was in 8.2.0 during the From e50fa6b839b2b87a9236ef71e216ff3e90d0405d Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Mon, 10 Jul 2017 14:28:33 +0200 Subject: [PATCH 2/4] async_hooks: make AsyncResource match emitInit AsyncResource previously called emitInitNative. Since AsyncResource is just an abstraction on top of the emitEventScript functions, it should call emitInitScript instead. --- lib/async_hooks.js | 27 +++++++------------ .../test-embedder.api.async-resource.js | 4 +-- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 7758c8d30577a0..39fd319fe54fe0 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -207,28 +207,16 @@ function triggerAsyncId() { // Embedder API // class AsyncResource { - constructor(type, triggerAsyncId) { - this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr]; - // Read and reset the current kInitTriggerId so that when the constructor - // finishes the kInitTriggerId field is always 0. - if (triggerAsyncId === undefined) { - triggerAsyncId = initTriggerId(); - // If a triggerAsyncId was passed, any kInitTriggerId still must be null'd. - } else { - async_uid_fields[kInitTriggerId] = 0; - } - this[trigger_id_symbol] = triggerAsyncId; - - if (typeof type !== 'string' || type.length <= 0) - throw new TypeError('type must be a string with length > 0'); + constructor(type, triggerAsyncId = initTriggerId()) { + // Unlike emitInitScript, AsyncResource doesn't supports null as the + // triggerAsyncId. if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0) throw new RangeError('triggerAsyncId must be an unsigned integer'); - // Return immediately if there's nothing to do. - if (async_hook_fields[kInit] === 0) - return; + this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr]; + this[trigger_id_symbol] = triggerAsyncId; - emitInitNative(this[async_id_symbol], type, triggerAsyncId, this); + emitInitScript(this[async_id_symbol], type, this[trigger_id_symbol], this); } emitBefore() { @@ -323,6 +311,9 @@ function emitInitScript(asyncId, type, triggerAsyncId, resource) { // manually means that the embedder must have used initTriggerId(). if (triggerAsyncId === null) { triggerAsyncId = initTriggerId(); + } else { + // If a triggerAsyncId was passed, any kInitTriggerId still must be null'd. + async_uid_fields[kInitTriggerId] = 0; } // TODO(trevnorris): I'd prefer allowing these checks to not exist, or only diff --git a/test/async-hooks/test-embedder.api.async-resource.js b/test/async-hooks/test-embedder.api.async-resource.js index 6574db8fffe1b6..2aec828c73892d 100644 --- a/test/async-hooks/test-embedder.api.async-resource.js +++ b/test/async-hooks/test-embedder.api.async-resource.js @@ -18,8 +18,8 @@ assert.throws(() => new AsyncResource('invalid_trigger_id', null), /^RangeError: triggerAsyncId must be an unsigned integer$/); assert.strictEqual( - typeof new AsyncResource('default_trigger_id').triggerAsyncId(), - 'number' + new AsyncResource('default_trigger_id').triggerAsyncId(), + async_hooks.executionAsyncId() ); // create first custom event 'alcazares' with triggerAsyncId derived From be5dbc5bdbddbbc50420ff4bf3f57be39f5bf617 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Wed, 12 Jul 2017 14:06:17 +0200 Subject: [PATCH 3/4] [squash] enable hooks when testing asserts --- ...tor.js => test-async-hooks-asyncresource-constructor.js} | 6 ++++++ 1 file changed, 6 insertions(+) rename test/parallel/{test-async-wrap-asyncresource-constructor.js => test-async-hooks-asyncresource-constructor.js} (82%) diff --git a/test/parallel/test-async-wrap-asyncresource-constructor.js b/test/parallel/test-async-hooks-asyncresource-constructor.js similarity index 82% rename from test/parallel/test-async-wrap-asyncresource-constructor.js rename to test/parallel/test-async-hooks-asyncresource-constructor.js index 2465f9590735ae..6e2aa2bc54e7f8 100644 --- a/test/parallel/test-async-wrap-asyncresource-constructor.js +++ b/test/parallel/test-async-hooks-asyncresource-constructor.js @@ -4,8 +4,14 @@ require('../common'); // This tests that AsyncResource throws an error if bad parameters are passed const assert = require('assert'); +const async_hooks = require('async_hooks'); const AsyncResource = require('async_hooks').AsyncResource; +// Setup init hook such parameters are validated +async_hooks.createHook({ + init() {} +}).enable(); + assert.throws(() => { return new AsyncResource(); }, /^TypeError: type must be a string with length > 0$/); From 11257914b23cc58c5ba3b8e9612bf846e0c195df Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Thu, 13 Jul 2017 09:39:13 +0200 Subject: [PATCH 4/4] [squash] fix double require --- test/parallel/test-async-hooks-asyncresource-constructor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-async-hooks-asyncresource-constructor.js b/test/parallel/test-async-hooks-asyncresource-constructor.js index 6e2aa2bc54e7f8..2fb0bb14ccf64e 100644 --- a/test/parallel/test-async-hooks-asyncresource-constructor.js +++ b/test/parallel/test-async-hooks-asyncresource-constructor.js @@ -5,7 +5,7 @@ require('../common'); const assert = require('assert'); const async_hooks = require('async_hooks'); -const AsyncResource = require('async_hooks').AsyncResource; +const { AsyncResource } = async_hooks; // Setup init hook such parameters are validated async_hooks.createHook({