From daacff858406c6afb95ad55a1108dddd419d313e Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Thu, 1 Feb 2018 15:25:41 -0800 Subject: [PATCH] async_hooks: deprecate unsafe emit{Before,After} The emit{Before,After} APIs in AsyncResource are problematic. * emit{Before,After} are named to suggest that the only thing they do is emit the before and after hooks. However, they in fact, mutate the current execution context. * They must be properly nested. Failure to do so by user code leads to catastrophic (unrecoverable) exceptions. It is very easy for the users to forget that they must be using a try/finally block around the code that must be surrounded by these operations. Even the example provided in the official docs makes this mistake. Failing to use a finally can lead to a catastrophic crash if the callback ends up throwing. This change provides a safer `runInAsyncScope` API as an alternative and deprecates emit{Before,After}. Backport-PR-URL: https://github.com/nodejs/node/pull/19517 PR-URL: https://github.com/nodejs/node/pull/18513 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Andreas Madsen --- doc/api/async_hooks.md | 64 +++++++++++++++---- doc/api/deprecations.md | 13 ++++ lib/async_hooks.js | 24 +++++++ ...dder.api.async-resource.runInAsyncScope.js | 11 ++++ ...c-hooks-recursive-stack-runInAsyncScope.js | 20 ++++++ ...fter-uncaught-exception-runInAsyncScope.js | 40 ++++++++++++ 6 files changed, 160 insertions(+), 12 deletions(-) create mode 100644 test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js create mode 100644 test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js create mode 100644 test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js diff --git a/doc/api/async_hooks.md b/doc/api/async_hooks.md index 56640060bf7616..c2abd5b78bade7 100644 --- a/doc/api/async_hooks.md +++ b/doc/api/async_hooks.md @@ -599,10 +599,6 @@ own resources. The `init` hook will trigger when an `AsyncResource` is instantiated. -*Note*: `before` and `after` calls must be unwound in the same order that they -are called. Otherwise, an unrecoverable exception will occur and the process -will abort. - The following is an overview of the `AsyncResource` API. ```js @@ -615,11 +611,13 @@ const asyncResource = new AsyncResource( type, { triggerAsyncId: executionAsyncId(), requireManualDestroy: false } ); -// Call AsyncHooks before callbacks. -asyncResource.emitBefore(); - -// Call AsyncHooks after callbacks. -asyncResource.emitAfter(); +// Run a function in the execution context of the resource. This will +// * establish the context of the resource +// * trigger the AsyncHooks before callbacks +// * call the provided function `fn` with the supplied arguments +// * trigger the AsyncHooks after callbacks +// * restore the original execution context +asyncResource.runInAsyncScope(fn, thisArg, ...args); // Call AsyncHooks destroy callbacks. asyncResource.emitDestroy(); @@ -629,6 +627,14 @@ asyncResource.asyncId(); // Return the trigger ID for the AsyncResource instance. asyncResource.triggerAsyncId(); + +// Call AsyncHooks before callbacks. +// Deprecated: Use asyncResource.runInAsyncScope instead. +asyncResource.emitBefore(); + +// Call AsyncHooks after callbacks. +// Deprecated: Use asyncResource.runInAsyncScope instead. +asyncResource.emitAfter(); ``` #### `AsyncResource(type[, options])` @@ -654,9 +660,7 @@ class DBQuery extends AsyncResource { getInfo(query, callback) { this.db.get(query, (err, data) => { - this.emitBefore(); - callback(err, data); - this.emitAfter(); + this.runInAsyncScope(callback, null, err, data); }); } @@ -667,7 +671,26 @@ class DBQuery extends AsyncResource { } ``` +#### `asyncResource.runInAsyncScope(fn[, thisArg, ...args])` + + +* `fn` {Function} The function to call in the execution context of this async + resource. +* `thisArg` {any} The receiver to be used for the function call. +* `...args` {any} Optional arguments to pass to the function. + +Call the provided function with the provided arguments in the execution context +of the async resource. This will establish the context, trigger the AsyncHooks +before callbacks, call the function, trigger the AsyncHooks after callbacks, and +then restore the original execution context. + #### `asyncResource.emitBefore()` + +> Stability: 0 - Deprecated: Use [`asyncResource.runInAsyncScope()`][] instead. * Returns: {undefined} @@ -675,7 +698,17 @@ Call all `before` callbacks to notify that a new asynchronous execution context is being entered. If nested calls to `emitBefore()` are made, the stack of `asyncId`s will be tracked and properly unwound. +`before` and `after` calls must be unwound in the same order that they +are called. Otherwise, an unrecoverable exception will occur and the process +will abort. For this reason, the `emitBefore` and `emitAfter` APIs are +considered deprecated. Please use `runInAsyncScope`, as it provides a much safer +alternative. + #### `asyncResource.emitAfter()` + +> Stability: 0 - Deprecated: Use [`asyncResource.runInAsyncScope()`][] instead. * Returns: {undefined} @@ -686,6 +719,12 @@ If the user's callback throws an exception, `emitAfter()` will automatically be called for all `asyncId`s on the stack if the error is handled by a domain or `'uncaughtException'` handler. +`before` and `after` calls must be unwound in the same order that they +are called. Otherwise, an unrecoverable exception will occur and the process +will abort. For this reason, the `emitBefore` and `emitAfter` APIs are +considered deprecated. Please use `runInAsyncScope`, as it provides a much safer +alternative. + #### `asyncResource.emitDestroy()` * Returns: {undefined} @@ -705,6 +744,7 @@ never be called. constructor. [`after` callback]: #async_hooks_after_asyncid +[`asyncResource.runInAsyncScope()`]: #async_hooks_asyncresource_runinasyncscope_fn_thisarg_args [`before` callback]: #async_hooks_before_asyncid [`destroy` callback]: #async_hooks_destroy_asyncid [`init` callback]: #async_hooks_init_asyncid_type_triggerasyncid_resource diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index a28df596d6919b..efa939c5779b55 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -684,6 +684,18 @@ Type: Runtime cause a lot of issues. See https://github.com/nodejs/node/issues/14328 for more details. + +### DEP0098: AsyncHooks Embedder AsyncResource.emit{Before,After} APIs + +Type: Runtime + +The embedded API provided by AsyncHooks exposes emit{Before,After} methods +which are very easy to use incorrectly which can lead to unrecoverable errors. + +Use [`asyncResource.runInAsyncScope()`][] API instead which provides a much +safer, and more convenient, alternative. See +https://github.com/nodejs/node/pull/18513 for more details. + [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array [`Buffer.from(buffer)`]: buffer.html#buffer_class_method_buffer_from_buffer @@ -694,6 +706,7 @@ details. [`Server.getConnections()`]: net.html#net_server_getconnections_callback [`Server.listen({fd: })`]: net.html#net_server_listen_handle_backlog_callback [`SlowBuffer`]: buffer.html#buffer_class_slowbuffer +[`asyncResource.runInAsyncScope()`]: async_hooks.html#async_hooks_asyncresource_runinasyncscope_fn_thisarg_args [`child_process`]: child_process.html [`console.error()`]: console.html#console_console_error_data_args [`console.log()`]: console.html#console_console_log_data_args diff --git a/lib/async_hooks.js b/lib/async_hooks.js index bf07358fd5e2c1..d527f7ea9d69c1 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -143,6 +143,17 @@ function triggerAsyncId() { const destroyedSymbol = Symbol('destroyed'); +let emitBeforeAfterWarning = true; +function showEmitBeforeAfterWarning() { + if (emitBeforeAfterWarning) { + process.emitWarning( + 'asyncResource.emitBefore and emitAfter are deprecated. Please use ' + + 'asyncResource.runInAsyncScope instead', + 'DeprecationWarning', 'DEP00XX'); + emitBeforeAfterWarning = false; + } +} + class AsyncResource { constructor(type, opts = {}) { if (typeof type !== 'string') @@ -178,15 +189,28 @@ class AsyncResource { } emitBefore() { + showEmitBeforeAfterWarning(); emitBefore(this[async_id_symbol], this[trigger_async_id_symbol]); return this; } emitAfter() { + showEmitBeforeAfterWarning(); emitAfter(this[async_id_symbol]); return this; } + runInAsyncScope(fn, thisArg, ...args) { + emitBefore(this[async_id_symbol], this[trigger_async_id_symbol]); + let ret; + try { + ret = Reflect.apply(fn, thisArg, args); + } finally { + emitAfter(this[async_id_symbol]); + } + return ret; + } + emitDestroy() { this[destroyedSymbol].destroyed = true; emitDestroy(this[async_id_symbol]); diff --git a/test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js b/test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js new file mode 100644 index 00000000000000..627880b4d98b07 --- /dev/null +++ b/test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js @@ -0,0 +1,11 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +// Ensure that asyncResource.makeCallback returns the callback return value. +const a = new async_hooks.AsyncResource('foobar'); +const ret = a.runInAsyncScope(() => { + return 1729; +}); +assert.strictEqual(ret, 1729); diff --git a/test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js b/test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js new file mode 100644 index 00000000000000..bc4ac86e7f1ca1 --- /dev/null +++ b/test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js @@ -0,0 +1,20 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +// This test verifies that the async ID stack can grow indefinitely. + +function recurse(n) { + const a = new async_hooks.AsyncResource('foobar'); + a.runInAsyncScope(() => { + assert.strictEqual(a.asyncId(), async_hooks.executionAsyncId()); + assert.strictEqual(a.triggerAsyncId(), async_hooks.triggerAsyncId()); + if (n >= 0) + recurse(n - 1); + assert.strictEqual(a.asyncId(), async_hooks.executionAsyncId()); + assert.strictEqual(a.triggerAsyncId(), async_hooks.triggerAsyncId()); + }); +} + +recurse(1000); diff --git a/test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js b/test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js new file mode 100644 index 00000000000000..5003972e9984aa --- /dev/null +++ b/test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js @@ -0,0 +1,40 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); + +const id_obj = {}; +let collect = true; + +const hook = async_hooks.createHook({ + before(id) { if (collect) id_obj[id] = true; }, + after(id) { delete id_obj[id]; }, +}).enable(); + +process.once('uncaughtException', common.mustCall((er) => { + assert.strictEqual(er.message, 'bye'); + collect = false; +})); + +setImmediate(common.mustCall(() => { + process.nextTick(common.mustCall(() => { + assert.strictEqual(Object.keys(id_obj).length, 0); + hook.disable(); + })); + + // Create a stack of async ids that will need to be emitted in the case of + // an uncaught exception. + const ar1 = new async_hooks.AsyncResource('Mine'); + ar1.runInAsyncScope(() => { + const ar2 = new async_hooks.AsyncResource('Mine'); + ar2.runInAsyncScope(() => { + throw new Error('bye'); + }); + }); + + // TODO(trevnorris): This test shows that the after() hooks are always called + // correctly, but it doesn't solve where the emitDestroy() is missed because + // of the uncaught exception. Simple solution is to always call emitDestroy() + // before the emitAfter(), but how to codify this? +}));