Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 7f5334e

Browse filesBrowse files
ofrobotsMylesBorins
ofrobots
authored and
MylesBorins
committed
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: #18892 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>
1 parent 5b88cb7 commit 7f5334e
Copy full SHA for 7f5334e

File tree

Expand file treeCollapse file tree

6 files changed

+160
-12
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+160
-12
lines changed
Open diff view settings
Collapse file

‎doc/api/async_hooks.md‎

Copy file name to clipboardExpand all lines: doc/api/async_hooks.md
+52-12Lines changed: 52 additions & 12 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -599,10 +599,6 @@ own resources.
599599

600600
The `init` hook will trigger when an `AsyncResource` is instantiated.
601601

602-
*Note*: `before` and `after` calls must be unwound in the same order that they
603-
are called. Otherwise, an unrecoverable exception will occur and the process
604-
will abort.
605-
606602
The following is an overview of the `AsyncResource` API.
607603

608604
```js
@@ -615,11 +611,13 @@ const asyncResource = new AsyncResource(
615611
type, { triggerAsyncId: executionAsyncId(), requireManualDestroy: false }
616612
);
617613

618-
// Call AsyncHooks before callbacks.
619-
asyncResource.emitBefore();
620-
621-
// Call AsyncHooks after callbacks.
622-
asyncResource.emitAfter();
614+
// Run a function in the execution context of the resource. This will
615+
// * establish the context of the resource
616+
// * trigger the AsyncHooks before callbacks
617+
// * call the provided function `fn` with the supplied arguments
618+
// * trigger the AsyncHooks after callbacks
619+
// * restore the original execution context
620+
asyncResource.runInAsyncScope(fn, thisArg, ...args);
623621

624622
// Call AsyncHooks destroy callbacks.
625623
asyncResource.emitDestroy();
@@ -629,6 +627,14 @@ asyncResource.asyncId();
629627

630628
// Return the trigger ID for the AsyncResource instance.
631629
asyncResource.triggerAsyncId();
630+
631+
// Call AsyncHooks before callbacks.
632+
// Deprecated: Use asyncResource.runInAsyncScope instead.
633+
asyncResource.emitBefore();
634+
635+
// Call AsyncHooks after callbacks.
636+
// Deprecated: Use asyncResource.runInAsyncScope instead.
637+
asyncResource.emitAfter();
632638
```
633639

634640
#### `AsyncResource(type[, options])`
@@ -654,9 +660,7 @@ class DBQuery extends AsyncResource {
654660

655661
getInfo(query, callback) {
656662
this.db.get(query, (err, data) => {
657-
this.emitBefore();
658-
callback(err, data);
659-
this.emitAfter();
663+
this.runInAsyncScope(callback, null, err, data);
660664
});
661665
}
662666

@@ -667,15 +671,44 @@ class DBQuery extends AsyncResource {
667671
}
668672
```
669673

674+
#### `asyncResource.runInAsyncScope(fn[, thisArg, ...args])`
675+
<!-- YAML
676+
added: REPLACEME
677+
-->
678+
679+
* `fn` {Function} The function to call in the execution context of this async
680+
resource.
681+
* `thisArg` {any} The receiver to be used for the function call.
682+
* `...args` {any} Optional arguments to pass to the function.
683+
684+
Call the provided function with the provided arguments in the execution context
685+
of the async resource. This will establish the context, trigger the AsyncHooks
686+
before callbacks, call the function, trigger the AsyncHooks after callbacks, and
687+
then restore the original execution context.
688+
670689
#### `asyncResource.emitBefore()`
690+
<!-- YAML
691+
deprecated: REPLACEME
692+
-->
693+
> Stability: 0 - Deprecated: Use [`asyncResource.runInAsyncScope()`][] instead.
671694
672695
* Returns: {undefined}
673696

674697
Call all `before` callbacks to notify that a new asynchronous execution context
675698
is being entered. If nested calls to `emitBefore()` are made, the stack of
676699
`asyncId`s will be tracked and properly unwound.
677700

701+
`before` and `after` calls must be unwound in the same order that they
702+
are called. Otherwise, an unrecoverable exception will occur and the process
703+
will abort. For this reason, the `emitBefore` and `emitAfter` APIs are
704+
considered deprecated. Please use `runInAsyncScope`, as it provides a much safer
705+
alternative.
706+
678707
#### `asyncResource.emitAfter()`
708+
<!-- YAML
709+
deprecated: REPLACEME
710+
-->
711+
> Stability: 0 - Deprecated: Use [`asyncResource.runInAsyncScope()`][] instead.
679712
680713
* Returns: {undefined}
681714

@@ -686,6 +719,12 @@ If the user's callback throws an exception, `emitAfter()` will automatically be
686719
called for all `asyncId`s on the stack if the error is handled by a domain or
687720
`'uncaughtException'` handler.
688721

722+
`before` and `after` calls must be unwound in the same order that they
723+
are called. Otherwise, an unrecoverable exception will occur and the process
724+
will abort. For this reason, the `emitBefore` and `emitAfter` APIs are
725+
considered deprecated. Please use `runInAsyncScope`, as it provides a much safer
726+
alternative.
727+
689728
#### `asyncResource.emitDestroy()`
690729

691730
* Returns: {undefined}
@@ -705,6 +744,7 @@ never be called.
705744
constructor.
706745

707746
[`after` callback]: #async_hooks_after_asyncid
747+
[`asyncResource.runInAsyncScope()`]: #async_hooks_asyncresource_runinasyncscope_fn_thisarg_args
708748
[`before` callback]: #async_hooks_before_asyncid
709749
[`destroy` callback]: #async_hooks_destroy_asyncid
710750
[`init` callback]: #async_hooks_init_asyncid_type_triggerasyncid_resource
Collapse file

‎doc/api/deprecations.md‎

Copy file name to clipboardExpand all lines: doc/api/deprecations.md
+13Lines changed: 13 additions & 0 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -768,6 +768,18 @@ Type: Runtime
768768
cause a lot of issues. See https://github.com/nodejs/node/issues/14328 for more
769769
details.
770770
771+
<a id="DEP0098"></a>
772+
### DEP0098: AsyncHooks Embedder AsyncResource.emit{Before,After} APIs
773+
774+
Type: Runtime
775+
776+
The embedded API provided by AsyncHooks exposes emit{Before,After} methods
777+
which are very easy to use incorrectly which can lead to unrecoverable errors.
778+
779+
Use [`asyncResource.runInAsyncScope()`][] API instead which provides a much
780+
safer, and more convenient, alternative. See
781+
https://github.com/nodejs/node/pull/18513 for more details.
782+
771783
[`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
772784
[`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
773785
[`Buffer.from(buffer)`]: buffer.html#buffer_class_method_buffer_from_buffer
@@ -777,6 +789,7 @@ details.
777789
[`Server.getConnections()`]: net.html#net_server_getconnections_callback
778790
[`Server.listen({fd: <number>})`]: net.html#net_server_listen_handle_backlog_callback
779791
[`SlowBuffer`]: buffer.html#buffer_class_slowbuffer
792+
[`asyncResource.runInAsyncScope()`]: async_hooks.html#async_hooks_asyncresource_runinasyncscope_fn_thisarg_args
780793
[`child_process`]: child_process.html
781794
[`console.error()`]: console.html#console_console_error_data_args
782795
[`console.log()`]: console.html#console_console_log_data_args
Collapse file

‎lib/async_hooks.js‎

Copy file name to clipboardExpand all lines: lib/async_hooks.js
+24Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,17 @@ function triggerAsyncId() {
143143

144144
const destroyedSymbol = Symbol('destroyed');
145145

146+
let emitBeforeAfterWarning = true;
147+
function showEmitBeforeAfterWarning() {
148+
if (emitBeforeAfterWarning) {
149+
process.emitWarning(
150+
'asyncResource.emitBefore and emitAfter are deprecated. Please use ' +
151+
'asyncResource.runInAsyncScope instead',
152+
'DeprecationWarning', 'DEP00XX');
153+
emitBeforeAfterWarning = false;
154+
}
155+
}
156+
146157
class AsyncResource {
147158
constructor(type, opts = {}) {
148159
if (typeof type !== 'string')
@@ -178,15 +189,28 @@ class AsyncResource {
178189
}
179190

180191
emitBefore() {
192+
showEmitBeforeAfterWarning();
181193
emitBefore(this[async_id_symbol], this[trigger_async_id_symbol]);
182194
return this;
183195
}
184196

185197
emitAfter() {
198+
showEmitBeforeAfterWarning();
186199
emitAfter(this[async_id_symbol]);
187200
return this;
188201
}
189202

203+
runInAsyncScope(fn, thisArg, ...args) {
204+
emitBefore(this[async_id_symbol], this[trigger_async_id_symbol]);
205+
let ret;
206+
try {
207+
ret = Reflect.apply(fn, thisArg, args);
208+
} finally {
209+
emitAfter(this[async_id_symbol]);
210+
}
211+
return ret;
212+
}
213+
190214
emitDestroy() {
191215
this[destroyedSymbol].destroyed = true;
192216
emitDestroy(this[async_id_symbol]);
Collapse file

‎test/async-hooks/test-embedder.api.async-resource.runInAsyncScope.js‎

Copy file name to clipboard
+11Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const async_hooks = require('async_hooks');
5+
6+
// Ensure that asyncResource.makeCallback returns the callback return value.
7+
const a = new async_hooks.AsyncResource('foobar');
8+
const ret = a.runInAsyncScope(() => {
9+
return 1729;
10+
});
11+
assert.strictEqual(ret, 1729);
Collapse file

‎test/parallel/test-async-hooks-recursive-stack-runInAsyncScope.js‎

Copy file name to clipboard
+20Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const async_hooks = require('async_hooks');
5+
6+
// This test verifies that the async ID stack can grow indefinitely.
7+
8+
function recurse(n) {
9+
const a = new async_hooks.AsyncResource('foobar');
10+
a.runInAsyncScope(() => {
11+
assert.strictEqual(a.asyncId(), async_hooks.executionAsyncId());
12+
assert.strictEqual(a.triggerAsyncId(), async_hooks.triggerAsyncId());
13+
if (n >= 0)
14+
recurse(n - 1);
15+
assert.strictEqual(a.asyncId(), async_hooks.executionAsyncId());
16+
assert.strictEqual(a.triggerAsyncId(), async_hooks.triggerAsyncId());
17+
});
18+
}
19+
20+
recurse(1000);
Collapse file

‎test/parallel/test-emit-after-uncaught-exception-runInAsyncScope.js‎

Copy file name to clipboard
+40Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const async_hooks = require('async_hooks');
6+
7+
const id_obj = {};
8+
let collect = true;
9+
10+
const hook = async_hooks.createHook({
11+
before(id) { if (collect) id_obj[id] = true; },
12+
after(id) { delete id_obj[id]; },
13+
}).enable();
14+
15+
process.once('uncaughtException', common.mustCall((er) => {
16+
assert.strictEqual(er.message, 'bye');
17+
collect = false;
18+
}));
19+
20+
setImmediate(common.mustCall(() => {
21+
process.nextTick(common.mustCall(() => {
22+
assert.strictEqual(Object.keys(id_obj).length, 0);
23+
hook.disable();
24+
}));
25+
26+
// Create a stack of async ids that will need to be emitted in the case of
27+
// an uncaught exception.
28+
const ar1 = new async_hooks.AsyncResource('Mine');
29+
ar1.runInAsyncScope(() => {
30+
const ar2 = new async_hooks.AsyncResource('Mine');
31+
ar2.runInAsyncScope(() => {
32+
throw new Error('bye');
33+
});
34+
});
35+
36+
// TODO(trevnorris): This test shows that the after() hooks are always called
37+
// correctly, but it doesn't solve where the emitDestroy() is missed because
38+
// of the uncaught exception. Simple solution is to always call emitDestroy()
39+
// before the emitAfter(), but how to codify this?
40+
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.