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}.

PR-URL: https://github.com/nodejs/node/pull/18513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

2 files changed, 65 insertions, 12 deletions

diff --git a/doc/api/async_hooks.md b/doc/api/async_hooks.md
index d854d737dc..4813169bfd 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.
 
-The `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])`
+<!-- YAML
+added: REPLACEME
+-->
+
+* `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()`
+<!-- YAML
+deprecated: REPLACEME
+-->
+> 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()`
+<!-- YAML
+deprecated: REPLACEME
+-->
+> 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 1f5809624a..c18c9d58e0 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -880,6 +880,18 @@ Users of `MakeCallback` that add the `domain` property to carry context,
 should start using the `async_context` variant of `MakeCallback` or
 `CallbackScope`, or the the high-level `AsyncResource` class.
 
+<a id="DEP0098"></a>
+### 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.
+
 [`--pending-deprecation`]: cli.html#cli_pending_deprecation
 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
 [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
@@ -892,6 +904,7 @@ should start using the `async_context` variant of `MakeCallback` or
 [`Server.getConnections()`]: net.html#net_server_getconnections_callback
 [`Server.listen({fd: <number>})`]: 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