async_hooks: make AsyncResource safer by ofrobots · Pull Request #18513 · nodejs/node (original) (raw)

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/