Skip to content

async_hooks: eliminate native PromiseHook

Rodrigo Muino Tomonari requested to merge github/fork/Qard/remove-native-promise-hooks into master

I've changed async_hooks use with a destroy hook to use the context-based PromiseHook now and trigger the destroy using the same registerDestroyHook logic that AsyncResource uses. This greatly simplifies the code for async_hooks and the AsyncWrap class.

I've deleted test/addons/async-hooks-promise/* as it's no longer a relevant test with PromiseWrap no longer being a thing.

I also uncovered a tiny inconsistency in that starting the context-based PromiseHook after the init of a promise and before attaching a promise.then(...) to it would result in the child promise having a lower async id than the parent promise due to order of operations in trackPromise(...). That has been fixed here. I can pull it out to a separate fix PR if this takes awhile to land.

Here's some benchmarks:

❯ ./node-master benchmark/async_hooks/promises.js
async_hooks/promises.js asyncHooks="enabled" n=1000000: 1,185,389.9728056605
async_hooks/promises.js asyncHooks="enabledWithDestroy" n=1000000: 203,357.96230288254
async_hooks/promises.js asyncHooks="enabledWithInitOnly" n=1000000: 1,090,968.40864669
async_hooks/promises.js asyncHooks="disabled" n=1000000: 1,823,403.2294724921
❯ ./node benchmark/async_hooks/promises.js       
async_hooks/promises.js asyncHooks="enabled" n=1000000: 1,192,662.4714112247
async_hooks/promises.js asyncHooks="enabledWithDestroy" n=1000000: 236,819.4646305771
async_hooks/promises.js asyncHooks="enabledWithInitOnly" n=1000000: 1,100,675.4615266216
async_hooks/promises.js asyncHooks="disabled" n=1000000: 1,809,663.626203757

Performance looks to be about the same or maybe slightly faster. The benefit is really just that it's a huge simplification of the code.

cc @nodejs/diagnostics

Merge request reports