async_hooks: make AsyncResource safer by ofrobots · Pull Request #18513 · nodejs/node · GitHub

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.

User code ends up becoming significantly safer, and cleaner as a result of the change here. Several foot guns have been removed.

N-API folks added a counterpart to this API which doesn't suffer from these foot-guns, and offers a makeCallback style approach that I am following here as well, albeit we're calling it runInAsyncScope as per discussion in this PR. I think we should be consistent between the JS and C++ versions of the API.

This would have been a semver major change, but async-hooks is still experimental, so we can do this as a semver-minor. Given the very strong likelihood of foot-gun in the user code with the existing API, I very strongly believe we should make this change and back-port this to 8.x as well. The transformation needed to react to the API change is trivial for correct user-space code

Given the very strong likelihood of foot-gun in the user code using the existing API, this PR runtime deprecates the dangerous APIs while providing a safe alternative. Since async-hooks is experimental, landing this would be semver-minor. This can be back-ported to 8.x and 9.x.

The emit{Before,After} APIs can be removed in a follow-on semver major change.

I have a very hard time imagining valid use-cases that this might break, but I am willing to be educated.

/cc @nodejs/diagnostics @nodejs/async_hooks

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

async_hooks

CI: https://ci.nodejs.org/job/node-test-pull-request/12899/
CI: https://ci.nodejs.org/job/node-test-pull-request/12910/
CI: https://ci.nodejs.org/job/node-test-pull-request/13016/
CI: https://ci.nodejs.org/job/node-test-pull-request/13027/

Is this example correct? err and data do not exist in this scope anymore... I could be missing something though

D'oh. Fixed :).

Nit: emit -> emitting?

User code ends up becoming significantly safer, and cleaner as a result of the change here. Several foot guns have been removed.

Yes, this is much better.

This would have been a semver major change, but async-hooks is still experimental, so we can do this as a semver-minor. Given the very strong likelihood of foot-gun in the user code with the existing API, I very strongly believe we should make this change and back-port this to 8.x as well. The transformation needed to react to the API change is trivial for correct user-space code.

Even though async_hooks is experimental we try to be nice when we can. I would like to see that emitBefore and emitAfter remains with an added deprecation in node 8.x and node 9.x.

Previously we have done that by first making a semver-minor PR that adds the depreciation and then follow that up with a semver-major PR that removes the deprecated API.

@AndreasMadsen That sounds reasonable. I have updated the PR so that it runtime deprecates the dangerous APIs rather than removing them. We can land this as a semver minor and back-port. They can be removed in a semver major follow-on.

Is there a reason to return this here rather than returning the return value of the function given to the Reflect.apply(...) call?

That is good feedback, thanks! I was following the fluent convention that the rest of the class uses, but you're correct that it is better to return value produced by the callback.

Done.

Missing bottom reference.

Done.

DEP00XX till landing?

Done.

It seems this should be
async_hooks.html#async_hooks_asyncresource_makecallback_fn_thisarg_args

Done.

Nit: can you just remove the *Note*: prefix here? :-)

Done.

Are we certain that there are not use-cases which require these to be separated?

@Fishrock123 I am willing to be educated if there are valid use-cases. If you have examples, please bring those to the table and let's evaluate.

@nodejs/async_hooks could I get another review for this?

LGTM

@addaleax Do we want to continue calling this makeCallback? :D

@AndreasMadsen I would personally prefer something like runInAsyncScope or runInCallbackScope, yeah…

@addaleax @AndreasMadsen I like those names 👍, with a slight lean towards runInAsyncScope, but we probably also want consistency between here and N-API (and Nan). If we're agreed on the name, then I can go add napi_run_in_async_scope to N-API as an alias for napi_make_callback after this PR lands. /cc @nodejs/addon-api

The CI is looking green enough. I plan on landing this on Friday.

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: nodejs#18513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) #18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) #18354 - ICU 60.2 bump (Steven R. Loomis) #17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) #16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) #15752 * http2: - add http fallback options to .createServer (Peter Marton) #15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) #16402 * inspector: - --inspect-brk for es modules (Guy Bedford) #18194 * lib: - allow process kill by signal number (Sam Roberts) #16944 * module: - enable dynamic import (Myles Borins) #18387 - dynamic import is now supported (Jan Krems) #15713 * napi: - add methods to open/close callback scope (Michael Dawson) #18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) #17600 * vm: - add support for es modules (Gus Caplan) #17560 PR-URL: #18902

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: nodejs#18513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Refs: nodejs#18513 PR-URL: nodejs#18694 Refs: nodejs#18513 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) nodejs#18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) nodejs#18354 - ICU 60.2 bump (Steven R. Loomis) nodejs#17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) nodejs#16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) nodejs#15752 * http2: - add http fallback options to .createServer (Peter Marton) nodejs#15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) nodejs#16402 * inspector: - --inspect-brk for es modules (Guy Bedford) nodejs#18194 * lib: - allow process kill by signal number (Sam Roberts) nodejs#16944 * module: - enable dynamic import (Myles Borins) nodejs#18387 - dynamic import is now supported (Jan Krems) nodejs#15713 * napi: - add methods to open/close callback scope (Michael Dawson) nodejs#18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) nodejs#17600 * vm: - add support for es modules (Gus Caplan) nodejs#17560 PR-URL: nodejs#18902

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: nodejs#19517 PR-URL: nodejs#18513 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Refs: nodejs#18513 Backport-PR-URL: nodejs#19517 PR-URL: nodejs#18694 Refs: nodejs#18513 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

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>

Refs: #18513 Backport-PR-URL: #19517 PR-URL: #18694 Refs: #18513 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Feb 1, 2018

ofrobots added semver-minor PRs that contain new features and should be released in the next minor version. v8.x labels Feb 1, 2018

evanlucas reviewed Feb 1, 2018

View reviewed changes

ofrobots added the v9.x label Feb 1, 2018

vsemozhetbyt reviewed Feb 1, 2018

View reviewed changes

ofrobots force-pushed the async-resource-make-callback branch from b0f2480 to 36ef3c7 Compare February 1, 2018 23:33

Qard reviewed Feb 2, 2018

View reviewed changes

vsemozhetbyt reviewed Feb 2, 2018

View reviewed changes

ofrobots force-pushed the async-resource-make-callback branch from 03c9e0c to bc19516 Compare February 2, 2018 16:18

jasnell reviewed Feb 2, 2018

View reviewed changes

jasnell approved these changes Feb 2, 2018

View reviewed changes

ofrobots mentioned this pull request Feb 3, 2018

Add async_hooks integration for gRPC Server grpc/grpc-node#169

Closed

addaleax approved these changes Feb 7, 2018

View reviewed changes

AndreasMadsen approved these changes Feb 7, 2018

View reviewed changes

ofrobots force-pushed the async-resource-make-callback branch from 0092bb1 to 46ee848 Compare February 7, 2018 16:35

ofrobots mentioned this pull request Feb 7, 2018

introduce AsyncResource class nodejs/nan#729

Merged

ofrobots force-pushed the async-resource-make-callback branch from f84a6b7 to 6a4f726 Compare February 9, 2018 21:00

ofrobots force-pushed the async-resource-make-callback branch from 6a4f726 to 523a155 Compare February 9, 2018 21:04

ofrobots mentioned this pull request Mar 21, 2018

[8.x] Backport 18513, 18694 #19517

Merged

4 tasks

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

async_hooks: make AsyncResource safer by ofrobots · Pull Request #18513 · nodejs/node · GitHub

Uh oh!

Uh oh!

async_hooks: make AsyncResource safer - #18513

async_hooks: make AsyncResource safer#18513ofrobots merged 1 commit intonodejs:masternodejs/node:masterfrom ofrobots:async-resource-make-callbackofrobots/node:async-resource-make-callbackCopy head branch name to clipboard

Conversation

ofrobots commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Feb 1, 2018

Uh oh!

ofrobots commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Feb 2, 2018

Uh oh!

ofrobots commented Feb 2, 2018

Uh oh!

ofrobots commented Feb 7, 2018

Uh oh!

AndreasMadsen left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 7, 2018

Uh oh!

ofrobots commented Feb 7, 2018

Uh oh!

ofrobots commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

async_hooks: make AsyncResource safer#18513
ofrobots merged 1 commit into
nodejs:masterfrom
ofrobots:async-resource-make-callback

ofrobots commented Feb 1, 2018 •
edited
Loading