Skip to content

Commit

Permalink
async_hooks: remove internal only error checking
Browse files Browse the repository at this point in the history 
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: #30967
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  • Loading branch information
@apapirovski @codebytere
apapirovski authored and codebytere committed Mar 17, 2020
1 parent 0f4a9e2 commit 597431b
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 110 deletions.
6 changes: 6 additions & 0 deletions lib/async_hooks.js
View file
Expand Up @@ -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');
Expand All @@ -32,6 +33,7 @@ const {
emitBefore,
emitAfter,
emitDestroy,
enabledHooksExist,
initHooksExist,
} = internal_async_hooks;

Expand Down Expand Up @@ -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);
}

Expand Down
42 changes: 5 additions & 37 deletions lib/internal/async_hooks.js
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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();
}
Expand All @@ -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)
Expand All @@ -367,8 +339,6 @@ function emitBeforeScript(asyncId, triggerAsyncId) {


function emitAfterScript(asyncId) {
validateAsyncId(asyncId, 'asyncId');

if (async_hook_fields[kAfter] > 0)
emitAfterNative(asyncId);

Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -464,6 +431,7 @@ module.exports = {
getOrSetAsyncId,
getDefaultTriggerAsyncId,
defaultTriggerAsyncIdScope,
enabledHooksExist,
initHooksExist,
afterHooksExist,
destroyHooksExist,
Expand Down
48 changes: 15 additions & 33 deletions test/async-hooks/test-emit-before-after.js
View file
Expand Up @@ -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';
Expand All @@ -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();

Expand Down
40 changes: 0 additions & 40 deletions test/async-hooks/test-emit-init.js
View file
Expand Up @@ -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');

Expand All @@ -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);

Expand Down

0 comments on commit 597431b

Please sign in to comment.