Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: remove internal only error checking #30967

Closed
wants to merge 3 commits into from
Closed

async_hooks: remove internal only error checking #30967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5dbd853
async_hooks: remove internal only error checking
apapirovski Dec 14, 2019
deb5e67
fixup
apapirovski Jan 8, 2020
b2b98e5
fixup
apapirovski Jan 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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();
}
Expand All @@ -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)
Expand All @@ -366,8 +338,6 @@ function emitBeforeScript(asyncId, triggerAsyncId) {


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

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

Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -463,6 +430,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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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