async_hooks: don't abort unnecessarily

* id values of -1 are allowed. They indicate that the id was never
  correctly assigned to the async resource. These will appear in any
  call graph, and will only be apparent to those using the async_hooks
  module, then reported in an issue.
* ids < -1 are still not allowed and will cause the application to
  exit the process; because there is no scenario where this should ever
  happen.
* Add asyncId range checks to emitAfterScript().
* Fix emitBeforeScript() range checks which should have been || not &&.
* Replace errors with entries in internal/errors.
* Fix async_hooks tests that check for exceptions to match new
  internal/errors entries.

NOTE: emit{Before,After,Destroy}() must continue to exit the process
because in the case of an exception during hook execution the state of
the application is unknowable. For example, an exception could cause a
memory leak:

    const id_map = new Map();

    before(id) {
      id_map.set(id, /* data object or similar */);
    },
    after(id) {
      throw new Error('id never dies!');
      id_map.delete(id);
    }

Allowing a recoverable exception may also cause an abort because of a
stack check in Environment::AsyncHooks::pop_ids() that verifies the
async id and pop'd ids match. This case would be more difficult to debug
than if fatalError() (lib/async_hooks.js) was called immediately.

    try {
      async_hooks.emitBefore(null, NaN);
    } catch (e) { }
    // do something
    async_hooks.emitAfter(5);

It also allows an edge case where emitBefore() could be called twice and
not have the pop_ids() CHECK fail:

    try {
      async_hooks.emitBefore(5, NaN);
    } catch (e) { }
    async_hooks.emitBefore(5);
    // do something
    async_hooks.emitAfter(5);

There is the option of allowing mismatches in the stack and ignoring the
check if no async hooks are enabled, but I don't believe going this far
is necessary.

PR-URL: https://github.com/nodejs/node/pull/14722
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>

3 files changed, 38 insertions, 18 deletions

diff --git a/test/async-hooks/test-embedder.api.async-resource.js b/test/async-hooks/test-embedder.api.async-resource.js
index 78985b955a..533df6a5c8 100644
--- a/test/async-hooks/test-embedder.api.async-resource.js
+++ b/test/async-hooks/test-embedder.api.async-resource.js
@@ -12,10 +12,18 @@ const { checkInvocations } = require('./hook-checks');
 const hooks = initHooks();
 hooks.enable();
 
-assert.throws(() => new AsyncResource(),
-              /^TypeError: type must be a string with length > 0$/);
-assert.throws(() => new AsyncResource('invalid_trigger_id', null),
-              /^RangeError: triggerAsyncId must be an unsigned integer$/);
+assert.throws(() => {
+  new AsyncResource();
+}, common.expectsError({
+  code: 'ERR_ASYNC_TYPE',
+  type: TypeError,
+}));
+assert.throws(() => {
+  new AsyncResource('invalid_trigger_id', null);
+}, common.expectsError({
+  code: 'ERR_INVALID_ASYNC_ID',
+  type: RangeError,
+}));
 
 assert.strictEqual(
   new AsyncResource('default_trigger_id').triggerAsyncId(),
diff --git a/test/async-hooks/test-emit-before-after.js b/test/async-hooks/test-emit-before-after.js
index 1334767c76..2b22739fa9 100644
--- a/test/async-hooks/test-emit-before-after.js
+++ b/test/async-hooks/test-emit-before-after.js
@@ -8,25 +8,25 @@ const initHooks = require('./init-hooks');
 
 switch (process.argv[2]) {
   case 'test_invalid_async_id':
-    async_hooks.emitBefore(-1, 1);
+    async_hooks.emitBefore(-2, 1);
     return;
   case 'test_invalid_trigger_id':
-    async_hooks.emitBefore(1, -1);
+    async_hooks.emitBefore(1, -2);
     return;
 }
 assert.ok(!process.argv[2]);
 
 
 const c1 = spawnSync(process.execPath, [__filename, 'test_invalid_async_id']);
-assert.strictEqual(c1.stderr.toString().split(/[\r\n]+/g)[0],
-                   'Error: before(): asyncId or triggerAsyncId is less than ' +
-                   'zero (asyncId: -1, triggerAsyncId: 1)');
+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, [__filename, 'test_invalid_trigger_id']);
-assert.strictEqual(c2.stderr.toString().split(/[\r\n]+/g)[0],
-                   'Error: before(): asyncId or triggerAsyncId is less than ' +
-                   'zero (asyncId: 1, triggerAsyncId: -1)');
+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.newUid();
diff --git a/test/async-hooks/test-emit-init.js b/test/async-hooks/test-emit-init.js
index feee3d944b..631dcd7599 100644
--- a/test/async-hooks/test-emit-init.js
+++ b/test/async-hooks/test-emit-init.js
@@ -25,12 +25,24 @@ const hooks1 = initHooks({
 
 hooks1.enable();
 
-assert.throws(() => async_hooks.emitInit(),
-              /^RangeError: asyncId must be an unsigned integer$/);
-assert.throws(() => async_hooks.emitInit(expectedId),
-              /^TypeError: type must be a string with length > 0$/);
-assert.throws(() => async_hooks.emitInit(expectedId, expectedType, -1),
-              /^RangeError: triggerAsyncId must be an unsigned integer$/);
+assert.throws(() => {
+  async_hooks.emitInit();
+}, common.expectsError({
+  code: 'ERR_INVALID_ASYNC_ID',
+  type: RangeError,
+}));
+assert.throws(() => {
+  async_hooks.emitInit(expectedId);
+}, common.expectsError({
+  code: 'ERR_INVALID_ASYNC_ID',
+  type: RangeError,
+}));
+assert.throws(() => {
+  async_hooks.emitInit(expectedId, expectedType, -2);
+}, common.expectsError({
+  code: 'ERR_INVALID_ASYNC_ID',
+  type: RangeError,
+}));
 
 async_hooks.emitInit(expectedId, expectedType, expectedTriggerId,
                      expectedResource);