async_hooks: remove experimental onPropagate option by jasnell · Pull Request #46386 · nodejs/node

jasnell · 2023-01-27T22:08:13Z

The onPropagate option for AsyncLocalStorage is problematic for a couple of reasons:

It is not expected to be forwards compatible in any way with the upcoming TC-39 AsyncContext proposal.
It introduces a non-trivial O(n) cost invoking a JavaScript callback for every AsyncResource that is created, including every Promise.

While it is still experimental, I recommend removing it while we can revisit the fundamental use cases in light of the coming AsyncContext proposal.

/cc @nodejs/async_hooks

Qard

LGTM, though @Flarna may have some thoughts as I believe they're depending on this interface. Not sure how critical it is but they may want a viable alternative proposed before this can be removed.

vdeturckheim

lgtm - aligned with @Qard here

GeoffreyBooth

I like the idea of aligning our development of this with WinterCG.

I don’t really accept that it’s our responsibility to develop replacements for any users depending on an experimental API. If they want a new feature in Node, they should propose and contribute it.

jasnell · 2023-01-28T02:07:09Z

If they want a new feature in Node, they should propose and contribute it.

Well, to be fair, that's exactly what @Flarna did :-) ... There's no rush pushing this out. We can give Flarna time to revisit if they want.

Qard · 2023-01-28T05:07:44Z

To be clear, I'm fine removing it as long as @Flarna doesn't feel it's too disruptive. I'm not aware of anyone else using it.

doc/api/async_context.md

Flarna · 2023-01-30T21:47:09Z

2. It introduces a non-trivial O(n) cost invoking a JavaScript callback for every AsyncResource that is created, including every Promise.

Actually the O(n) cost is not introduced by onPropagate. If set each resource will trigger one call. O(n) will only happen if there are n ALS instances using onPropagate.
The O(n) complexity is already there independent of onPropagate because ALS uses the init hook to propagate context internally in all ALS instances.

Anyhow, following TC-39 sounds reasonable. Any hints that TC-39 will accept the AsyncContext proposal anytime soon? As far as I know the proposal is a few years old meanwhile and has still not reached stage 1. Or is there something ongoing which is not visible on the TC-39 propsals github repo?

lib/async_hooks.js

jasnell · 2023-01-30T21:53:58Z

Anyhow, following TC-39 sounds reasonable. Any hints that TC-39 will accept the AsyncContext proposal anytime soon? As far as I know the proposal is a few years old meanwhile and has still not reached stage 1. Or is there something ongoing which is not visible on the TC-39 propsals github repo?

Work is underway now. With @jridgewell, @legendecas, and @littledan heading it up.

Refs: nodejs#46374 The `onPropagate` option for `AsyncLocalStorage` is problematic for a couple of reasons: 1. It is not expected to be forwards compatible in any way with the upcoming TC-39 `AsyncContext` proposal. 2. It introduces a non-trivial O(n) cost invoking a JavaScript callback for *every* AsyncResource that is created, including every Promise. While it is still experimental, I recommend removing it while we can revisit the fundamental use cases in light of the coming `AsyncContext` proposal.

nodejs-github-bot · 2023-01-30T22:49:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49267/

jridgewell · 2023-01-30T23:20:30Z

Anyhow, following TC-39 sounds reasonable. Any hints that TC-39 will accept the AsyncContext proposal anytime soon?

We'll have a better idea of how long it will take after the TC39 meeting this week.

For now, I think it's better to think of this as aligning with the implementation strategy that AsyncContext will take, which will make everything considerably faster. It's possible that V8 will add a special hook for Node to control how the propagation works (they same way they've added support for async_hooks), but that has the potential to impact the performance guarantees that AsyncContext requires to be part of the core language.

As far as I know the proposal is a few years old meanwhile and has still not reached stage 1. Or is there something ongoing which is not visible on the TC-39 propsals github repo?

I've been meeting with delegates for a few weeks getting feedback on the proposal. We've gotten approval from the security folks, which was a major blocker the last time this was presented. We also have a private matrix channel to discuss that will become public when we get accepted to Stage 1.

legendecas · 2023-01-31T02:18:29Z

The O(n) complexity is already there independent of onPropagate because ALS uses the init hook to propagate context internally in all ALS instances.

Value propagation can be optimized to avoid the complexity with the number of AsyncLocalStorage instances, as the propagation doesn't calling into user code. See https://github.com/legendecas/proposal-async-context/blob/master/src/index.ts#L7 for an example.

nodejs-github-bot · 2023-01-31T02:24:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49270/

nodejs-github-bot · 2023-02-01T15:20:22Z

Commit Queue failed

- Loading data for nodejs/node/pull/46386
✔  Done loading data for nodejs/node/pull/46386
----------------------------------- PR info ------------------------------------
Title      async_hooks: remove experimental onPropagate option (#46386)
Author     James M Snell  (@jasnell)
Branch     jasnell:remove-asynclocalstorage-onpropagate -> nodejs:main
Labels     async_hooks, author ready
Commits    1
 - async_hooks: remove experimental onPropagate option
Committers 1
 - James M Snell 
PR-URL: https://github.com/nodejs/node/pull/46386
Refs: https://github.com/nodejs/node/issues/46374
Reviewed-By: Stephen Belanger 
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Geoffrey Booth 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Chengzhong Wu 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46386
Refs: https://github.com/nodejs/node/issues/46374
Reviewed-By: Stephen Belanger 
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Geoffrey Booth 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Chengzhong Wu 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 27 Jan 2023 22:08:13 GMT
   ✔  Approvals: 6
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/46386#pullrequestreview-1273599442
   ✔  - Vladimir de Turckheim (@vdeturckheim): https://github.com/nodejs/node/pull/46386#pullrequestreview-1273608580
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/46386#pullrequestreview-1273724192
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/46386#pullrequestreview-1273975929
   ✔  - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/46386#pullrequestreview-1276087836
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/46386#pullrequestreview-1276289905
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-01-31T02:24:58Z: https://ci.nodejs.org/job/node-test-pull-request/49270/
- Querying data for job/node-test-pull-request/49270/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 46386
From https://github.com/nodejs/node
 * branch                  refs/pull/46386/merge -> FETCH_HEAD
✔  Fetched commits as 088e470dcdaa..3c265fa0aace
--------------------------------------------------------------------------------
[main 60f40eb7a8] async_hooks: remove experimental onPropagate option
 Author: James M Snell 
 Date: Fri Jan 27 14:03:51 2023 -0800
 3 files changed, 6 insertions(+), 79 deletions(-)
 delete mode 100644 test/async-hooks/test-async-local-storage-stop-propagation.js
   ✔  Patches applied
--------------------------------------------------------------------------------
   ✖  Git found no trailers in the original commit message, but 'Refs: https://github.com/nodejs/node/issues/46374' is present and should be a trailer.

https://github.com/nodejs/node/actions/runs/4065850372

The `onPropagate` option for `AsyncLocalStorage` is problematic for a couple of reasons: 1. It is not expected to be forwards compatible in any way with the upcoming TC-39 `AsyncContext` proposal. 2. It introduces a non-trivial O(n) cost invoking a JavaScript callback for *every* AsyncResource that is created, including every Promise. While it is still experimental, I recommend removing it while we can revisit the fundamental use cases in light of the coming `AsyncContext` proposal. Refs: #46374 PR-URL: #46386 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

jasnell · 2023-02-01T15:37:34Z

Landed in dc90810

The `onPropagate` option for `AsyncLocalStorage` is problematic for a couple of reasons: 1. It is not expected to be forwards compatible in any way with the upcoming TC-39 `AsyncContext` proposal. 2. It introduces a non-trivial O(n) cost invoking a JavaScript callback for *every* AsyncResource that is created, including every Promise. While it is still experimental, I recommend removing it while we can revisit the fundamental use cases in light of the coming `AsyncContext` proposal. Refs: #46374 PR-URL: #46386 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

jasnell requested review from Flarna and Qard January 27, 2023 22:08

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. needs-ci PRs that need a full CI run. labels Jan 27, 2023

Qard approved these changes Jan 27, 2023

View reviewed changes

vdeturckheim approved these changes Jan 27, 2023

View reviewed changes

GeoffreyBooth approved these changes Jan 28, 2023

View reviewed changes

benjamingr approved these changes Jan 28, 2023

View reviewed changes

benjamingr reviewed Jan 28, 2023

View reviewed changes

doc/api/async_context.md Outdated Show resolved Hide resolved

cjihrig reviewed Jan 28, 2023

View reviewed changes

doc/api/async_context.md Outdated Show resolved Hide resolved

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2023

This comment was marked as outdated.

Sign in to view

Flarna approved these changes Jan 30, 2023

View reviewed changes

Flarna reviewed Jan 30, 2023

View reviewed changes

lib/async_hooks.js Show resolved Hide resolved

Flarna added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 30, 2023

jasnell removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jan 30, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2023

This comment was marked as outdated.

Sign in to view

jasnell force-pushed the remove-asynclocalstorage-onpropagate branch from c08df91 to 3c265fa Compare January 30, 2023 21:59

jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 30, 2023

legendecas approved these changes Jan 31, 2023

View reviewed changes

tony-go approved these changes Jan 31, 2023

View reviewed changes

jasnell added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Feb 1, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 1, 2023

jasnell closed this Feb 1, 2023

Flarna mentioned this pull request Feb 1, 2023

Revisit the AsyncLocalStorage onPropagate option #46374

Closed

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

Uh oh!

Conversation

jasnell commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

vdeturckheim left a comment

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 28, 2023

Uh oh!

Qard commented Jan 28, 2023

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Flarna commented Jan 30, 2023

Uh oh!

Uh oh!

jasnell commented Jan 30, 2023

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jan 30, 2023

Uh oh!

jridgewell commented Jan 30, 2023

Uh oh!

legendecas commented Jan 31, 2023

Uh oh!

nodejs-github-bot commented Jan 31, 2023

Uh oh!

nodejs-github-bot commented Feb 1, 2023

Uh oh!

jasnell commented Feb 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jasnell commented Jan 27, 2023 •

edited

Loading