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.
apapirovski · Dec 14, 2019 · Jan 8, 2020 · Jan 8, 2020 · Dec 14, 2019 · 5dbd8537640ef39d7f03b6af6f520e505ffaf93a
commit 5dbd8537640ef39d7f03b6af6f520e505ffaf93a
diff --git 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
@@ -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
@@ -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
@@ -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);