| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this 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.
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
| @@ -0,0 +1,86 @@ | |||
| # Async Storage | |||
|
|
|||
| <!--introduced_in=v12.x.x--> | |||
There was a problem hiding this comment.
is this supposed to be added: REPLACEME ? Does introduced_in work also?
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Should we create npm placeholder for this name as per our guide? |
Sorry, something went wrong.
|
Go ahead. I kind of expect this to go under `@nodejs/async-storage` but
better safe than sorry.
Il giorno sab 9 mar 2019 alle ore 13:10 Vse Mozhet Byt <
notifications@github.com> ha scritto:
… Should we create npm placeholder for this name as per our guides?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#26540 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AADL4xdIdUUD6v4zX5DYe6C_E1y5GCinks5vU6SmgaJpZM4bmmI4>
.
|
Sorry, something went wrong.
|
Sorry, I am not experienced in registering placeholders in npm, so anybody more confident please do it. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Does this require a separate new top level module? Could this be exposed alternatively through the existing async_hooks module? Or via util? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
|
if this is going to be in core I'd want it to be more similar to https://github.com/WICG/kv-storage |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
|
I think it would be better placed in async_hooks instead of util. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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
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
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
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
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
|
Any chance this will be backported to v12? |
Sorry, something went wrong.
It would be great to have AsyncLocalStorage in v12, so I'm all for backporting it. Note: executionAsyncResource (#30959) has to be backported first. |
Sorry, something went wrong.
|
As far as I know, backporting executionAsyncResource to v12 should not be a problem, but I haven't tested it myself. |
Sorry, something went wrong.
|
Care must be taken to include also the followup PRs fixing various bugs found afterwards. |
Sorry, something went wrong.
|
I've created a backport PR for executionAsyncResource: #32131. Hopefully, it includes all bugfixes. |
Sorry, something went wrong.
|
@puzpuzpuz thanks! this next release is a patch so it'll go out in the subsequent minor. |
Sorry, something went wrong.
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>
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>
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>
|
Providing a reference to another issue here because most interested parties are subscribed to this issue. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This introduces a new API in core to provide asynchronous storage features.
More and more Node.js users have been needing such API to:
Some userland modules exist but they:
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