From 597431b1f22629010b02ac2ac55f2fec32db6191 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Sat, 14 Dec 2019 15:02:50 -0500 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/30967 Reviewed-By: James M Snell Reviewed-By: Stephen Belanger Reviewed-By: Rich Trott Reviewed-By: Ruben Bridgewater --- lib/async_hooks.js | 6 +++ lib/internal/async_hooks.js | 42 +++---------------- test/async-hooks/test-emit-before-after.js | 48 +++++++--------------- test/async-hooks/test-emit-init.js | 40 ------------------ 4 files changed, 26 insertions(+), 110 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 0b599869ef1be2..009f15c83b1d8e 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 f6fb19c996f30b..02b0e91ac2e965 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -3,16 +3,10 @@ const { Error, 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 @@ -117,15 +111,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 @@ -314,6 +299,9 @@ function defaultTriggerAsyncIdScope(triggerAsyncId, block, ...args) { } } +function enabledHooksExist() { + return async_hook_fields[kCheck] > 0; +} function initHooksExist() { return async_hook_fields[kInit] > 0; @@ -329,21 +317,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(); } @@ -353,12 +331,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) @@ -367,8 +339,6 @@ function emitBeforeScript(asyncId, triggerAsyncId) { function emitAfterScript(asyncId) { - validateAsyncId(asyncId, 'asyncId'); - if (async_hook_fields[kAfter] > 0) emitAfterNative(asyncId); @@ -377,8 +347,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; @@ -418,8 +386,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); } @@ -464,6 +431,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..2ad98b993efd5b 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'; @@ -45,9 +14,22 @@ 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)), + onbefore: checkOnce(chkBefore), + onafter: checkOnce(chkAfter), allowNoInit: true }).enable(); 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);