async_hooks: eliminate native PromiseHook by Qard · Pull Request #39135 · nodejs/node · GitHub

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

@Qard I know the SES group doesn't want .domain to be attached to Promises in VMs due to concerns about using it to crawl the object graph to escape some sandboxing.

Yep, that was my finding when I looked up the history of that test file. Not yet sure why that property has returned. I think it'll take a bit of code spelunking to figure out how it's actually suppose to be preventing that currently.

PR-URL: #39135 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Promise handling is special-cased for domains and trace_events. Domains must not add the `domain` property to promises in other contexts, and trace_events must be emitted from JavaScript as promises no longer have an AsyncWrap type attached to them. PR-URL: #39135 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

PR-URL: nodejs#39135 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Promise handling is special-cased for domains and trace_events. Domains must not add the `domain` property to promises in other contexts, and trace_events must be emitted from JavaScript as promises no longer have an AsyncWrap type attached to them. PR-URL: nodejs#39135 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

PR-URL: #39135 Backport-PR-URL: #39742 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Promise handling is special-cased for domains and trace_events. Domains must not add the `domain` property to promises in other contexts, and trace_events must be emitted from JavaScript as promises no longer have an AsyncWrap type attached to them. PR-URL: #39135 Backport-PR-URL: #39742 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

PR-URL: #39135 Backport-PR-URL: #39742 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

FYI I believe this PR causes issues with SES users and debuggers. See #42229

I'm not sure why, but before this change, attaching a debugger with async stack support didn't cause promise objects to be modified by async_hooks (while still allowing async stack traces to work in the debugger). After this change, promise objects are expanded with the async hooks symbols, which fails if the promise object is frozen.

Qard added c++ Issues and PRs that require attention from people who are familiar with C++. async_wrap async_hooks Issues and PRs related to the async hooks subsystem. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Jun 24, 2021

github-actions Bot added the needs-ci PRs that need a full CI run. label Jun 24, 2021

Qard force-pushed the remove-native-promise-hooks branch 3 times, most recently from 751cd24 to 799303b Compare June 24, 2021 06:51

Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2021

This comment has been minimized.

Sign in to view

Hbutlercapone approved these changes Jun 26, 2021

View reviewed changes

Qard force-pushed the remove-native-promise-hooks branch from 799303b to 666983d Compare July 5, 2021 21:13

Stephen Belanger added 2 commits July 5, 2021 15:16

async_hooks: eliminate native PromiseHook

cb99784

async_hooks: emit promise trace events from JS

e469ca6

Qard force-pushed the remove-native-promise-hooks branch from ad65c19 to e1e008d Compare July 5, 2021 22:16

This comment has been minimized.

Sign in to view

domain: do not add domain to promise from other context

f76d251

Qard force-pushed the remove-native-promise-hooks branch from e1e008d to f76d251 Compare July 5, 2021 22:20

Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2021

This comment has been minimized.

Sign in to view

Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2021

This comment has been minimized.

Sign in to view

Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2021

This comment has been minimized.

Sign in to view

BethGriggs pushed a commit that referenced this pull request Jul 29, 2021

async_hooks: eliminate native PromiseHook

e18778d

PR-URL: #39135 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

BethGriggs pushed a commit that referenced this pull request Jul 29, 2021

async_hooks: emit promise trace events from JS

b5248d4

PR-URL: #39135 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Qard mentioned this pull request Aug 12, 2021

[v14.x backport] async_hooks: eliminate native PromiseHook #39742

Closed

Flarna added the backport-open-v14.x label Aug 21, 2021

targos added backported-to-v14.x and removed backport-open-v14.x labels Aug 29, 2021

targos mentioned this pull request Sep 4, 2021

v14.18.0 release proposal #39990

Merged

mhofman mentioned this pull request Mar 8, 2022

Async hooks cause programs with frozen promises to crash #42229

Closed

mhofman mentioned this pull request Mar 15, 2022

Handle symbols installed on Promise by Node's async_hooks endojs/endo#1115

Merged

legendecas mentioned this pull request May 10, 2022

Tracking issue for failure on crash of CallbackScope::Close on testing nodejs/node-addon-api#979

Closed

joyeecheung mentioned this pull request Aug 26, 2023

src: remove unused PromiseWrap-related code #49335

Merged

Flarna mentioned this pull request Nov 28, 2024

src: remove dead code in async_wrap #56065

Merged

async_hooks: eliminate native PromiseHook by Qard · Pull Request #39135 · nodejs/node · GitHub

Uh oh!

Uh oh!

async_hooks: eliminate native PromiseHook - #39135

async_hooks: eliminate native PromiseHook#39135
Qard wants to merge 4 commits into
nodejs:masterfrom
Qard:remove-native-promise-hooks

Qard commented Jun 24, 2021 •
edited
Loading

Uh oh!

Uh oh!

This comment has been minimized.

bmeck commented Jun 26, 2021

Uh oh!

Qard commented Jun 26, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mhofman commented Mar 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

async_hooks: eliminate native PromiseHook by Qard · Pull Request #39135 · nodejs/node · GitHub

Uh oh!

Uh oh!

async_hooks: eliminate native PromiseHook - #39135

async_hooks: eliminate native PromiseHook#39135Qard wants to merge 4 commits intonodejs:masternodejs/node:masterfrom Qard:remove-native-promise-hooksQard/node:remove-native-promise-hooksCopy head branch name to clipboard

Conversation

Qard commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

bmeck commented Jun 26, 2021

Uh oh!

Qard commented Jun 26, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mhofman commented Mar 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

async_hooks: eliminate native PromiseHook#39135
Qard wants to merge 4 commits into
nodejs:masterfrom
Qard:remove-native-promise-hooks

Qard commented Jun 24, 2021 •
edited
Loading