Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async-hooks: introduce async-storage API #26540

Closed
wants to merge 1 commit into from

Conversation

@vdeturckheim
Copy link
Member

vdeturckheim commented Mar 9, 2019

This introduces a new API in core to provide asynchronous storage features.

More and more Node.js users have been needing such API to:

  • build monitoring tools
  • build features that enhence code readability and reduce the need of passing HTTP request objects all the way to one single method that needs it (for instance, a logger).

Some userland modules exist but they:

  • sometimes badly interact with each other
  • introduce potential easy mistakes in usage (need to be required first)
  • might need some specific addons to handle part of the ecosystem properly

Havin such API in core will make the ecosystem more stable and reliable when needing an asynchronous storage.

This is still an early work and there is probably a lot of updates to do to this PR :)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
lib/async_storage.js Outdated Show resolved Hide resolved
lib/async_storage.js Outdated Show resolved Hide resolved
lib/async_storage.js Outdated Show resolved Hide resolved
lib/async_storage.js Outdated Show resolved Hide resolved
Copy link
Contributor

mscdex left a comment

IMO this is best left as a userland module, especially considering it can be fairly easily implemented using just async_hooks. There are also some assumptions being made design-wise here that others might disagree with and would want more flexibility.

Even if this kind of feature were to be added to node core, it would be better to have it start in userland to allow easier iteration on the idea first, to figure out the best API and behaviors between all parties that would be involved.

doc/api/async_storage.md Outdated Show resolved Hide resolved
doc/api/async_storage.md Outdated Show resolved Hide resolved
doc/api/async_storage.md Outdated Show resolved Hide resolved
lib/async_storage.js Outdated Show resolved Hide resolved
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Mar 9, 2019

@mscdex This was talked about at the diagnostics summit. I don’t think the feature has to exist in this particular form, but having it in core made sense in general, and in particular performance-wise, since a good implementation could allow us to skip calling async_hooks callbacks.

FYI @nodejs/tsc

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Mar 9, 2019

I think this functionality belongs in core. We should offer a single way to do cls that is high performant, stable and reliable. It’s not an ecosystem concern, it’s a runtime concern (as much as a ThreadLocal implementation is for Java/.NET/.. etc).

@@ -0,0 +1,86 @@
# Async Storage

<!--introduced_in=v12.x.x-->

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 9, 2019

Member

is this supposed to be added: REPLACEME ? Does introduced_in work also?

This comment has been minimized.

Copy link
@addaleax

addaleax Mar 9, 2019

Member

I think it’s a separate system used for the per-page “View another version” dropdown in the generated documentation:

image

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 9, 2019

I agree that this should be in core and there was consensus among the @nodejs/diagnostics WG members attending the diagnostics summit. Sure, we can bikeshed around the specific API, and I would like to get some of the APM vendors to review this before it lands, it would be generally better to have a single authoritative API and implementation of this than multiple userland implementations so that APM solutions built on top can be implemented consistently.

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

vsemozhetbyt commented Mar 9, 2019

Should we create npm placeholder for this name as per our guide?

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Mar 9, 2019

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

vsemozhetbyt commented Mar 9, 2019

Sorry, I am not experienced in registering placeholders in npm, so anybody more confident please do it.

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Mar 9, 2019

There is https://www.npmjs.com/package/async-storage, which makes calling this module async_storage not a great idea. @nodejs/async_storage is ok.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 9, 2019

Does this require a separate new top level module? Could this be exposed alternatively through the existing async_hooks module? Or via util?

@vdeturckheim

This comment has been minimized.

Copy link
Member Author

vdeturckheim commented Mar 9, 2019

@jasnell I actually think it would make sense to have that in util too. I am just worried potential future features would make this API big enough for us to regret it was not a module. That said, I'd be happy to move this to another module.

@vdeturckheim

This comment has been minimized.

Copy link
Member Author

vdeturckheim commented Mar 9, 2019

I will update the PR over the week end and start to contact APM and RASP vendors to ensure thay can comment.

Thanks a lot to all of you for the comments and suggestions!

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Mar 9, 2019

if this is going to be in core I'd want it to be more similar to https://github.com/WICG/kv-storage

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 9, 2019

@vdeturckheim ... One thing to consider is that by putting this into a new module, it automatically becomes semver-major and significantly harder to backport. My hope is that we could at least get this into 10.x also.

@vdeturckheim

This comment has been minimized.

Copy link
Member Author

vdeturckheim commented Mar 9, 2019

@jasnell good point, I'll move it to util then (this is a killer argument for me indeed ;) )

@devsnek I did not know about this proposal. I'd be interested in other opinions here but the solutions I could see would probably be a bit slow.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Mar 9, 2019

@vdeturckheim reading over this again it seems i was confused as to what this pr is adding to node.

i think it might lend itself better to a different name like "async_closure" or "async_context" something.

i'd be strongly in favor of making the class available as an async_hooks.Context

@targos

This comment has been minimized.

Copy link
Member

targos commented Mar 9, 2019

I think it would be better placed in async_hooks instead of util.

@devsnek devsnek added the async_hooks label Mar 9, 2019
@othiym23

This comment has been minimized.

Copy link
Contributor

othiym23 commented Mar 9, 2019

As a developer and co-maintainer of continuation-local-storage, I’m very happy to see this functionality coming to Node core, and am amused to see how positive the reception is this time. A lot can change in 6 years. If anyone wants to reuse the name, or join this with the existing continuation-local-storage, I’m happy to hand the name over to the Foundation, although I understand that this implementation is simpler to use than CLS.

@Trott Trott removed the tsc-review label Mar 10, 2019
@vdeturckheim vdeturckheim changed the title async-storage: introduce async-storage API async-hooks: introduce async-storage API Mar 10, 2019
@targos

This comment has been minimized.

Copy link
Member

targos commented Feb 24, 2020

Should wait for #31930 before this is backported to v13.x

@vdeturckheim vdeturckheim mentioned this pull request Feb 25, 2020
4 of 4 tasks complete
@wentout wentout mentioned this pull request Feb 25, 2020
codebytere added a commit that referenced this pull request Feb 27, 2020
Adding AsyncLocalStorage class to async_hooks
 module.
This API provide a simple CLS-like set
of features.

Co-authored-by: Andrey Pechkurov <apechkurov@gmail.com>

PR-URL: #26540
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@vdeturckheim

This comment has been minimized.

Copy link
Member Author

vdeturckheim commented Feb 27, 2020

Should wait for #31930 before this is backported to v13.x

#31930 landed, good to go to v13.x

@puzpuzpuz puzpuzpuz mentioned this pull request Feb 29, 2020
3 of 3 tasks complete
@codebytere codebytere mentioned this pull request Feb 29, 2020
codebytere added a commit that referenced this pull request Mar 1, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 4, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
@Sytten

This comment has been minimized.

Copy link

Sytten commented Mar 6, 2020

Any chance this will be backported to v12?

@puzpuzpuz

This comment has been minimized.

Copy link
Contributor

puzpuzpuz commented Mar 6, 2020

Any chance this will be backported to v12?

It would be great to have AsyncLocalStorage in v12, so I'm all for backporting it.

Note: executionAsyncResource (#30959) has to be backported first.

cc @Qard @vdeturckheim

@Qard

This comment has been minimized.

Copy link
Member

Qard commented Mar 6, 2020

As far as I know, backporting executionAsyncResource to v12 should not be a problem, but I haven't tested it myself.

@Flarna

This comment has been minimized.

Copy link
Member

Flarna commented Mar 6, 2020

Care must be taken to include also the followup PRs fixing various bugs found afterwards.

@puzpuzpuz

This comment has been minimized.

Copy link
Contributor

puzpuzpuz commented Mar 6, 2020

I've created a backport PR for executionAsyncResource: #32131. Hopefully, it includes all bugfixes.

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Mar 16, 2020

@puzpuzpuz thanks! this next release is a patch so it'll go out in the subsequent minor.

puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Mar 17, 2020
Adding AsyncLocalStorage class to async_hooks
 module.
This API provide a simple CLS-like set
of features.

Co-authored-by: Andrey Pechkurov <apechkurov@gmail.com>

PR-URL: nodejs#26540
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@puzpuzpuz

This comment has been minimized.

Copy link
Contributor

puzpuzpuz commented Mar 17, 2020

v12 backport: #32318

puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 2, 2020
Adding AsyncLocalStorage class to async_hooks
 module.
This API provide a simple CLS-like set
of features.

Co-authored-by: Andrey Pechkurov <apechkurov@gmail.com>

PR-URL: nodejs#26540
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.