Skip to content

Commit

Permalink
async_hooks: deprecate unsafe emit{Before,After}
Browse files Browse the repository at this point in the history
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: #19517
PR-URL: #18513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
  • Loading branch information
ofrobots authored and rvagg committed Aug 16, 2018
1 parent f987a51 commit daacff8
Show file tree
Hide file tree
Showing 6 changed files with 160 additions and 12 deletions.
64 changes: 52 additions & 12 deletions doc/api/async_hooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand All @@ -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])`
Expand All @@ -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);
});
}

Expand All @@ -667,15 +671,44 @@ 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}

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}

Expand All @@ -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}
Expand All @@ -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
Expand Down
13 changes: 13 additions & 0 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,18 @@ Type: Runtime
cause a lot of issues. See https://github.com/nodejs/node/issues/14328 for more
details.
<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.
[`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
Expand All @@ -694,6 +706,7 @@ details.
[`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
Expand Down
24 changes: 24 additions & 0 deletions lib/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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]);
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
20 changes: 20 additions & 0 deletions test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js
Original file line number Diff line number Diff line change
@@ -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);
Original file line number Diff line number Diff line change
@@ -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?
}));

0 comments on commit daacff8

Please sign in to comment.