From 5dbd8537640ef39d7f03b6af6f520e505ffaf93a Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski <anatoli.papirovski@postmates.com> Date: Sat, 14 Dec 2019 15:02:50 -0500 Subject: [PATCH 1/3] async_hooks: remove internal only error checking This error checking is mostly unnecessary and is just a Node core developer nicety, rather than something that is needed for the user-land. It can be safely removed without any practical impact while making nextTick, timers, immediates and AsyncResource substantially faster. --- lib/async_hooks.js | 6 ++++ lib/internal/async_hooks.js | 42 +++------------------- test/async-hooks/test-emit-before-after.js | 31 ---------------- test/async-hooks/test-emit-init.js | 40 --------------------- 4 files changed, 11 insertions(+), 108 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index be32f6d1102bbd..be44744993f6a1 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -8,6 +8,7 @@ const { const { ERR_ASYNC_CALLBACK, + ERR_ASYNC_TYPE, ERR_INVALID_ASYNC_ID } = require('internal/errors').codes; const { validateString } = require('internal/validators'); @@ -32,6 +33,7 @@ const { emitBefore, emitAfter, emitDestroy, + enabledHooksExist, initHooksExist, } = internal_async_hooks; @@ -158,6 +160,10 @@ class AsyncResource { this[trigger_async_id_symbol] = triggerAsyncId; if (initHooksExist()) { + if (enabledHooksExist() && type.length === 0) { + throw new ERR_ASYNC_TYPE(type); + } + emitInit(asyncId, type, triggerAsyncId, this); } diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index bf5456a858870f..087554fece4192 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -2,16 +2,10 @@ const { FunctionPrototypeBind, - NumberIsSafeInteger, ObjectDefineProperty, Symbol, } = primordials; -const { - ERR_ASYNC_TYPE, - ERR_INVALID_ASYNC_ID -} = require('internal/errors').codes; - const async_wrap = internalBinding('async_wrap'); /* async_hook_fields is a Uint32Array wrapping the uint32_t array of * Environment::AsyncHooks::fields_[]. Each index tracks the number of active @@ -116,15 +110,6 @@ function fatalError(e) { } -function validateAsyncId(asyncId, type) { - // Skip validation when async_hooks is disabled - if (async_hook_fields[kCheck] <= 0) return; - - if (!NumberIsSafeInteger(asyncId) || asyncId < -1) { - fatalError(new ERR_INVALID_ASYNC_ID(type, asyncId)); - } -} - // Emit From Native // // Used by C++ to call all init() callbacks. Because some state can be setup @@ -313,6 +298,9 @@ function defaultTriggerAsyncIdScope(triggerAsyncId, block, ...args) { } } +function enabledHooksExist() { + return async_hook_fields[kCheck] > 0; +} function initHooksExist() { return async_hook_fields[kInit] > 0; @@ -328,21 +316,11 @@ function destroyHooksExist() { function emitInitScript(asyncId, type, triggerAsyncId, resource) { - validateAsyncId(asyncId, 'asyncId'); - if (triggerAsyncId !== null) - validateAsyncId(triggerAsyncId, 'triggerAsyncId'); - if (async_hook_fields[kCheck] > 0 && - (typeof type !== 'string' || type.length <= 0)) { - throw new ERR_ASYNC_TYPE(type); - } - // 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). if (async_hook_fields[kInit] === 0) return; - // This can run after the early return check b/c running this function - // manually means that the embedder must have used getDefaultTriggerAsyncId(). if (triggerAsyncId === null) { triggerAsyncId = getDefaultTriggerAsyncId(); } @@ -352,12 +330,6 @@ function emitInitScript(asyncId, type, triggerAsyncId, resource) { function emitBeforeScript(asyncId, triggerAsyncId) { - // Validate the ids. An id of -1 means it was never set and is visible on the - // call graph. An id < -1 should never happen in any circumstance. Throw - // on user calls because async state should still be recoverable. - validateAsyncId(asyncId, 'asyncId'); - validateAsyncId(triggerAsyncId, 'triggerAsyncId'); - pushAsyncIds(asyncId, triggerAsyncId); if (async_hook_fields[kBefore] > 0) @@ -366,8 +338,6 @@ function emitBeforeScript(asyncId, triggerAsyncId) { function emitAfterScript(asyncId) { - validateAsyncId(asyncId, 'asyncId'); - if (async_hook_fields[kAfter] > 0) emitAfterNative(asyncId); @@ -376,8 +346,6 @@ function emitAfterScript(asyncId) { function emitDestroyScript(asyncId) { - validateAsyncId(asyncId, 'asyncId'); - // Return early if there are no destroy callbacks, or invalid asyncId. if (async_hook_fields[kDestroy] === 0 || asyncId <= 0) return; @@ -417,8 +385,7 @@ function popAsyncIds(asyncId) { const stackLength = async_hook_fields[kStackLength]; if (stackLength === 0) return false; - if (async_hook_fields[kCheck] > 0 && - async_id_fields[kExecutionAsyncId] !== asyncId) { + if (enabledHooksExist() && async_id_fields[kExecutionAsyncId] !== asyncId) { // Do the same thing as the native code (i.e. crash hard). return popAsyncIds_(asyncId); } @@ -463,6 +430,7 @@ module.exports = { getOrSetAsyncId, getDefaultTriggerAsyncId, defaultTriggerAsyncIdScope, + enabledHooksExist, initHooksExist, afterHooksExist, destroyHooksExist, diff --git a/test/async-hooks/test-emit-before-after.js b/test/async-hooks/test-emit-before-after.js index 6a9ceaeefb8c0a..431489a9e07e39 100644 --- a/test/async-hooks/test-emit-before-after.js +++ b/test/async-hooks/test-emit-before-after.js @@ -3,40 +3,9 @@ const common = require('../common'); const assert = require('assert'); -const spawnSync = require('child_process').spawnSync; const async_hooks = require('internal/async_hooks'); const initHooks = require('./init-hooks'); -if (!common.isMainThread) - common.skip('Worker bootstrapping works differently -> different async IDs'); - -switch (process.argv[2]) { - case 'test_invalid_async_id': - async_hooks.emitBefore(-2, 1); - return; - case 'test_invalid_trigger_id': - async_hooks.emitBefore(1, -2); - return; -} -assert.ok(!process.argv[2]); - - -const c1 = spawnSync(process.execPath, [ - '--expose-internals', __filename, 'test_invalid_async_id' -]); -assert.strictEqual( - c1.stderr.toString().split(/[\r\n]+/g)[0], - 'RangeError [ERR_INVALID_ASYNC_ID]: Invalid asyncId value: -2'); -assert.strictEqual(c1.status, 1); - -const c2 = spawnSync(process.execPath, [ - '--expose-internals', __filename, 'test_invalid_trigger_id' -]); -assert.strictEqual( - c2.stderr.toString().split(/[\r\n]+/g)[0], - 'RangeError [ERR_INVALID_ASYNC_ID]: Invalid triggerAsyncId value: -2'); -assert.strictEqual(c2.status, 1); - const expectedId = async_hooks.newAsyncId(); const expectedTriggerId = async_hooks.newAsyncId(); const expectedType = 'test_emit_before_after_type'; diff --git a/test/async-hooks/test-emit-init.js b/test/async-hooks/test-emit-init.js index f5d5687cb4d11d..bc1cd49cba2824 100644 --- a/test/async-hooks/test-emit-init.js +++ b/test/async-hooks/test-emit-init.js @@ -3,7 +3,6 @@ const common = require('../common'); const assert = require('assert'); -const spawnSync = require('child_process').spawnSync; const async_hooks = require('internal/async_hooks'); const initHooks = require('./init-hooks'); @@ -23,45 +22,6 @@ const hooks1 = initHooks({ hooks1.enable(); -switch (process.argv[2]) { - case 'test_invalid_async_id': - async_hooks.emitInit(); - return; - case 'test_invalid_trigger_id': - async_hooks.emitInit(expectedId); - return; - case 'test_invalid_trigger_id_negative': - async_hooks.emitInit(expectedId, expectedType, -2); - return; -} -assert.ok(!process.argv[2]); - - -const c1 = spawnSync(process.execPath, [ - '--expose-internals', __filename, 'test_invalid_async_id' -]); -assert.strictEqual( - c1.stderr.toString().split(/[\r\n]+/g)[0], - 'RangeError [ERR_INVALID_ASYNC_ID]: Invalid asyncId value: undefined'); -assert.strictEqual(c1.status, 1); - -const c2 = spawnSync(process.execPath, [ - '--expose-internals', __filename, 'test_invalid_trigger_id' -]); -assert.strictEqual( - c2.stderr.toString().split(/[\r\n]+/g)[0], - 'RangeError [ERR_INVALID_ASYNC_ID]: Invalid triggerAsyncId value: undefined'); -assert.strictEqual(c2.status, 1); - -const c3 = spawnSync(process.execPath, [ - '--expose-internals', __filename, 'test_invalid_trigger_id_negative' -]); -assert.strictEqual( - c3.stderr.toString().split(/[\r\n]+/g)[0], - 'RangeError [ERR_INVALID_ASYNC_ID]: Invalid triggerAsyncId value: -2'); -assert.strictEqual(c3.status, 1); - - async_hooks.emitInit(expectedId, expectedType, expectedTriggerId, expectedResource); From deb5e67d051ce4ce8b3afdc3700b10ce60fa1b70 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski <anatoli.papirovski@postmates.com> Date: Tue, 7 Jan 2020 19:25:41 -0800 Subject: [PATCH 2/3] fixup --- test/async-hooks/test-emit-before-after.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/test/async-hooks/test-emit-before-after.js b/test/async-hooks/test-emit-before-after.js index 431489a9e07e39..dad35531e030c4 100644 --- a/test/async-hooks/test-emit-before-after.js +++ b/test/async-hooks/test-emit-before-after.js @@ -14,9 +14,23 @@ const expectedType = 'test_emit_before_after_type'; async_hooks.emitBefore(expectedId, expectedTriggerId); async_hooks.emitAfter(expectedId); +const chkBefore = common.mustCall((id) => assert.strictEqual(id, expectedId)); +const chkAfter = common.mustCall((id) => assert.strictEqual(id, expectedId)); + +const checkOnce = (fn) => { + let called = false; + return (...args) => { + if (called) return; + + called = true; + fn(...args); + }; +}; + initHooks({ - onbefore: common.mustCall((id) => assert.strictEqual(id, expectedId)), - onafter: common.mustCall((id) => assert.strictEqual(id, expectedId)), + oninit: (id, type) => process.stdout.write(`${id} ${type}`), + onbefore: checkOnce(chkBefore), + onafter: checkOnce(chkAfter), allowNoInit: true }).enable(); From b2b98e5c9b3637243a772f290d40d70179066a42 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski <anatoli.papirovski@postmates.com> Date: Tue, 7 Jan 2020 19:36:12 -0800 Subject: [PATCH 3/3] fixup --- test/async-hooks/test-emit-before-after.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/async-hooks/test-emit-before-after.js b/test/async-hooks/test-emit-before-after.js index dad35531e030c4..2ad98b993efd5b 100644 --- a/test/async-hooks/test-emit-before-after.js +++ b/test/async-hooks/test-emit-before-after.js @@ -28,7 +28,6 @@ const checkOnce = (fn) => { }; initHooks({ - oninit: (id, type) => process.stdout.write(`${id} ${type}`), onbefore: checkOnce(chkBefore), onafter: checkOnce(chkAfter), allowNoInit: true