diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 38bdbb0306b6be..67e81ecaecb28a 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -2,6 +2,7 @@ const internalUtil = require('internal/util'); const async_wrap = process.binding('async_wrap'); +const errors = require('internal/errors'); /* async_hook_fields is a Uint32Array wrapping the uint32_t array of * Environment::AsyncHooks::fields_[]. Each index tracks the number of active * hooks for each type. @@ -109,13 +110,13 @@ function fatalError(e) { class AsyncHook { constructor({ init, before, after, destroy }) { if (init !== undefined && typeof init !== 'function') - throw new TypeError('init must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'init'); if (before !== undefined && typeof before !== 'function') - throw new TypeError('before must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before'); if (after !== undefined && typeof after !== 'function') - throw new TypeError('after must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before'); if (destroy !== undefined && typeof destroy !== 'function') - throw new TypeError('destroy must be a function'); + throw new errors.TypeError('ERR_ASYNC_CALLBACK', 'before'); this[init_symbol] = init; this[before_symbol] = before; @@ -236,8 +237,11 @@ class AsyncResource { constructor(type, triggerAsyncId = initTriggerId()) { // Unlike emitInitScript, AsyncResource doesn't supports null as the // triggerAsyncId. - if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0) - throw new RangeError('triggerAsyncId must be an unsigned integer'); + if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) { + throw new errors.RangeError('ERR_INVALID_ASYNC_ID', + 'triggerAsyncId', + triggerAsyncId); + } this[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr]; this[trigger_id_symbol] = triggerAsyncId; @@ -342,14 +346,17 @@ function emitInitScript(asyncId, type, triggerAsyncId, resource) { async_uid_fields[kInitTriggerId] = 0; } - // TODO(trevnorris): I'd prefer allowing these checks to not exist, or only - // throw in a debug build, in order to improve performance. - if (!Number.isSafeInteger(asyncId) || asyncId < 0) - throw new RangeError('asyncId must be an unsigned integer'); - if (typeof type !== 'string' || type.length <= 0) - throw new TypeError('type must be a string with length > 0'); - if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < 0) - throw new RangeError('triggerAsyncId must be an unsigned integer'); + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + throw new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId); + } + if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) { + throw new errors.RangeError('ERR_INVALID_ASYNC_ID', + 'triggerAsyncId', + triggerAsyncId); + } + if (typeof type !== 'string' || type.length <= 0) { + throw new errors.TypeError('ERR_ASYNC_TYPE', type); + } emitInitNative(asyncId, type, triggerAsyncId, resource); } @@ -393,13 +400,17 @@ function emitHookFactory(symbol, name) { function emitBeforeScript(asyncId, triggerAsyncId) { - // CHECK(Number.isSafeInteger(asyncId) && asyncId > 0) - // CHECK(Number.isSafeInteger(triggerAsyncId) && triggerAsyncId > 0) - - // Validate the ids. - if (asyncId < 0 || triggerAsyncId < 0) { - fatalError('before(): asyncId or triggerAsyncId is less than zero ' + - `(asyncId: ${asyncId}, triggerAsyncId: ${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. + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + if (!Number.isSafeInteger(triggerAsyncId) || triggerAsyncId < -1) { + fatalError(new errors.RangeError('ERR_INVALID_ASYNC_ID', + 'triggerAsyncId', + triggerAsyncId)); } pushAsyncIds(asyncId, triggerAsyncId); @@ -410,6 +421,11 @@ function emitBeforeScript(asyncId, triggerAsyncId) { function emitAfterScript(asyncId) { + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + if (async_hook_fields[kAfter] > 0) emitAfterNative(asyncId); @@ -418,9 +434,13 @@ function emitAfterScript(asyncId) { function emitDestroyScript(asyncId) { - // Return early if there are no destroy callbacks, or on attempt to emit - // destroy on the void. - if (async_hook_fields[kDestroy] === 0 || asyncId === 0) + if (!Number.isSafeInteger(asyncId) || asyncId < -1) { + fatalError( + new errors.RangeError('ERR_INVALID_ASYNC_ID', 'asyncId', asyncId)); + } + + // Return early if there are no destroy callbacks, or invalid asyncId. + if (async_hook_fields[kDestroy] === 0 || asyncId <= 0) return; async_wrap.addIdToDestroyList(asyncId); } diff --git a/lib/internal/errors.js b/lib/internal/errors.js index b7dd509070731d..2b06583dd483e3 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -109,6 +109,8 @@ module.exports = exports = { // Note: Please try to keep these in alphabetical order E('ERR_ARG_NOT_ITERABLE', '%s must be iterable'); E('ERR_ASSERTION', (msg) => msg); +E('ERR_ASYNC_CALLBACK', (name) => `${name} must be a function`); +E('ERR_ASYNC_TYPE', (s) => `Invalid name for async "type": ${s}`); E('ERR_ENCODING_INVALID_ENCODED_DATA', (enc) => `The encoded data was not valid for encoding ${enc}`); E('ERR_ENCODING_NOT_SUPPORTED', @@ -184,6 +186,7 @@ E('ERR_HTTP2_UNSUPPORTED_PROTOCOL', (protocol) => `protocol "${protocol}" is unsupported.`); E('ERR_INDEX_OUT_OF_RANGE', 'Index out of range'); E('ERR_INVALID_ARG_TYPE', invalidArgType); +E('ERR_INVALID_ASYNC_ID', (type, id) => `Invalid ${type} value: ${id}`); E('ERR_INVALID_CALLBACK', 'callback must be a function'); E('ERR_INVALID_FD', (fd) => `"fd" must be a positive integer: ${fd}`); E('ERR_INVALID_FILE_URL_HOST', 'File URL host %s'); diff --git a/src/env-inl.h b/src/env-inl.h index 87fe7e36a2f001..69b5a5e858b458 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -127,8 +127,8 @@ inline v8::Local Environment::AsyncHooks::provider_string(int idx) { inline void Environment::AsyncHooks::push_ids(double async_id, double trigger_id) { - CHECK_GE(async_id, 0); - CHECK_GE(trigger_id, 0); + CHECK_GE(async_id, -1); + CHECK_GE(trigger_id, -1); ids_stack_.push({ uid_fields_[kCurrentAsyncId], uid_fields_[kCurrentTriggerId] }); @@ -181,6 +181,7 @@ inline Environment::AsyncHooks::InitScope::InitScope( Environment* env, double init_trigger_id) : env_(env), uid_fields_ref_(env->async_hooks()->uid_fields()) { + CHECK_GE(init_trigger_id, -1); env->async_hooks()->push_ids(uid_fields_ref_[AsyncHooks::kCurrentAsyncId], init_trigger_id); } @@ -194,6 +195,8 @@ inline Environment::AsyncHooks::ExecScope::ExecScope( : env_(env), async_id_(async_id), disposed_(false) { + CHECK_GE(async_id, -1); + CHECK_GE(trigger_id, -1); env->async_hooks()->push_ids(async_id, trigger_id); } diff --git a/test/async-hooks/test-embedder.api.async-resource.js b/test/async-hooks/test-embedder.api.async-resource.js index 78985b955ad683..533df6a5c84549 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 1334767c766a25..2b22739fa9478d 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 feee3d944b8afb..631dcd759968cb 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); diff --git a/test/parallel/test-async-hooks-asyncresource-constructor.js b/test/parallel/test-async-hooks-asyncresource-constructor.js index 2fb0bb14ccf64e..2ab5f067ca6c76 100644 --- a/test/parallel/test-async-hooks-asyncresource-constructor.js +++ b/test/parallel/test-async-hooks-asyncresource-constructor.js @@ -1,8 +1,8 @@ 'use strict'; -require('../common'); // This tests that AsyncResource throws an error if bad parameters are passed +const common = require('../common'); const assert = require('assert'); const async_hooks = require('async_hooks'); const { AsyncResource } = async_hooks; @@ -14,16 +14,28 @@ async_hooks.createHook({ assert.throws(() => { return new AsyncResource(); -}, /^TypeError: type must be a string with length > 0$/); +}, common.expectsError({ + code: 'ERR_ASYNC_TYPE', + type: TypeError, +})); assert.throws(() => { new AsyncResource(''); -}, /^TypeError: type must be a string with length > 0$/); +}, common.expectsError({ + code: 'ERR_ASYNC_TYPE', + type: TypeError, +})); assert.throws(() => { new AsyncResource('type', -4); -}, /^RangeError: triggerAsyncId must be an unsigned integer$/); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); assert.throws(() => { new AsyncResource('type', Math.PI); -}, /^RangeError: triggerAsyncId must be an unsigned integer$/); +}, common.expectsError({ + code: 'ERR_INVALID_ASYNC_ID', + type: RangeError, +})); diff --git a/test/parallel/test-async-wrap-constructor.js b/test/parallel/test-async-wrap-constructor.js index 4f344fd99bcff4..86fce0e3f39e68 100644 --- a/test/parallel/test-async-wrap-constructor.js +++ b/test/parallel/test-async-wrap-constructor.js @@ -1,8 +1,8 @@ 'use strict'; -require('../common'); // This tests that using falsy values in createHook throws an error. +const common = require('../common'); const assert = require('assert'); const async_hooks = require('async_hooks'); @@ -10,6 +10,9 @@ for (const badArg of [0, 1, false, true, null, 'hello']) { for (const field of ['init', 'before', 'after', 'destroy']) { assert.throws(() => { async_hooks.createHook({ [field]: badArg }); - }, new RegExp(`^TypeError: ${field} must be a function$`)); + }, common.expectsError({ + code: 'ERR_ASYNC_CALLBACK', + type: TypeError, + })); } }