async_hooks: remove internal only error checking

apapirovski · MylesBorins · commit 9d26358cfe72 · 2020-01-16T01:39:13.000-05:00
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>
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');
@@ -31,6 +32,7 @@ const {
   emitBefore,
   emitAfter,
   emitDestroy,
+  enabledHooksExist,
   initHooksExist,
 } = internal_async_hooks;
 
@@ -157,6 +159,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
@@ -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
@@ -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
@@ -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);
 
