async_hooks: stabilize part of AsyncLocalStorage by vdeturckheim · Pull Request #37675 · nodejs/node · GitHub

This PR stabilizes part of the AsyncLocalStorage API.

The whole reasoning for chosing this subset of methods was discussed in #35286 (comment)

cc @bmeck @Qard @mhdawson @mcollina @bengl

LGTM

There is still a Stability: 1 - Experimental on top for the whole module.

Maybe we should remove it there and add it instead at Class: AsyncHook and Class: AsyncResource?
Don't know who this is usually handled.

lgtm

There is still a Stability: 1 - Experimental on top for the whole module.
Maybe we should remove it there and add it instead at Class: AsyncHook and Class: AsyncResource?
Don't know who this is usually handled.

@Flarna , I don't either ^^ Maybe someone over @nodejs/documentation knows what's the best way to do it here?

@vdeturckheim what about creating new page2 for AsyncResouce and AsyncLocalStorage ?

I think that makes sense as they are each a unique concept/api (although related) on their own, and if for example we ended up deprecating the lower level async hooks they could still stand on their own.

I also think that would be less confusing in terms of what is experimental or not as well as being able to directly reference AsyncResouce

I'm a little -0 on giving them their own import specifier / page. I think having all these diagnostic and power tools in a single place really lets the organization be closer to purpose rather than purely on API stability. I'm fine if a new page is done, but would be -1 on changing the import specifier unless there was a clearer checklist of what should cause such a change for other APIs in the future.

I was not suggesting changing the import specifier, just the organization of the docs.

👍 for splitting up the docs. If we're splitting the doc up here anyway, perhaps we should also include AsyncResource in the changes too while you're at it?

@Qard I don't have any problem with having AsyncResource in the PR too :)

@mhdawson good idea!

I'll update the PR

Trying to do it, should I just add a new page named async_context.md and titled "Asynchronous Context Tracking"?
wdyt @mhdawson @Qard ?

@vdeturckheim that sounds reasonable to me.

So both AsyncLocalStorage and AsyncResource on the same page? Makes sense to me. Might actually want to make it a bit more guide-like than some of the other pages, with extra front content explaining the use and caveats like context loss in more detail, then have the reference content follow.

I am converting the PR into a draft as I think there might be a couple back and forth until we find the right format for the split doc. @Qard @mhdawson here is a split proposal (with a few parts to finish ofc)

@vdeturckheim being split out looks good. I'm thinking it might make sense to put the AsyncLocalStorage part first as it seems like the simpler of the two, and one of the key reasons you need AsyncResource is to keep AsyncLocalStorage working properly in a subset of the cases so seems like a good thing to follow AsyncLocalStorage. WDYT? Just a suggestion as I'd be good either way once the TODO's are filled in.

@mhdawson agreed. I updated a bit. It should start to look good now.

Looks like a good start. A few thoughts:

Should AsyncLocalStorage and AsyncResource get separate pages, and related--should we aim to make them separate top-level modules eventually?

Should the requireManualDestroy option and emitDestroy method of AsyncResource remain experimental? They're perhaps a bit janky as far as features go, and one could draw some parallels to the unsafety of domains in there, though I'm not too worried about it. Perhaps just a warning about timing of the destroy?

For the intro, it would be good to elaborate on the relationship between AsyncLocalStorage on the consuming end and AsyncResource on the producing end of async context propagation semantics.

Co-authored-by: Michaël Zasso <targos@protonmail.com> Co-authored-by: Andrey Pechkurov <37772591+puzpuzpuz@users.noreply.github.com>

Signed-off-by: Michael Dawson <mdawson@devrus.com>

Doc only, so full CI not required.

@bmeck @Qard @mcollina @bengl @vdeturckheim FYI as long as the github actions all pass I'll plan to land this tomorrow in case you want to take another look.

I think I've fixed up all the things I talked with @vdeturckheim about (unless there is more that the actions run which make test locally does not run).

@mhdawson I think this is blocked by #38781. We should label this "stable" after we fix that regression.

@mcollina thanks for the heads up.

@mhdawson #38821 has landed :)

I had some comments on doc/api/async_context.md, but only after I had reviewed it and prepared them all did I realize that that page wasn't written in this PR, it was only moved. 😆 I'll submit my suggestions in a new PR once this is merged.

lgtm

Mark AsyncLocalStorage constructor, AsyncLocalStorage.prototype.getStore(), and AsyncLocalStorage.prototype.run as stable. PR-URL: #37675 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com>

Landed in 7612d82

@vdeturckheim thanks for all your work on this.

Mark AsyncLocalStorage constructor, AsyncLocalStorage.prototype.getStore(), and AsyncLocalStorage.prototype.run as stable. PR-URL: #37675 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com>

Notable changes: * async_hooks: * stabilize part of AsyncLocalStorage (Vladimir de Turckheim) #37675 * deps: * upgrade npm to 7.18.1 (npm-robot) #39065 * update V8 to 9.1.269.36 (Michaël Zasso) #38273 * dns: * allow `--dns-result-order` to change default dns verbatim (Ouyang Yadong) #38099 PR-URL: #39031

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations. labels Mar 9, 2021

vdeturckheim requested review from Qard and puzpuzpuz March 9, 2021 14:09

bmeck approved these changes Mar 9, 2021

View reviewed changes

puzpuzpuz approved these changes Mar 9, 2021

View reviewed changes

vdeturckheim mentioned this pull request Mar 9, 2021

Mongodb breaks AsyncLocalStorage context spread nodejs/help#3186

Closed

mhdawson added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 9, 2021

mhdawson approved these changes Mar 9, 2021

View reviewed changes

Flarna reviewed Mar 9, 2021

View reviewed changes

Comment thread doc/api/async_hooks.md Outdated

Qard approved these changes Mar 9, 2021

View reviewed changes

mcollina approved these changes Mar 9, 2021

View reviewed changes

cjihrig approved these changes Mar 9, 2021

View reviewed changes

Flarna approved these changes Mar 10, 2021

View reviewed changes

aymen94 approved these changes Mar 11, 2021

View reviewed changes

legendecas approved these changes Mar 17, 2021

View reviewed changes

vdeturckheim marked this pull request as draft March 19, 2021 14:02

Qard reviewed Mar 25, 2021

View reviewed changes

Comment thread doc/api/async_context.md Outdated

Comment thread doc/api/async_context.md Outdated

vdeturckheim and others added 4 commits May 25, 2021 19:57

Apply suggestions from code review

7cd0c5c

Co-authored-by: Michaël Zasso <targos@protonmail.com> Co-authored-by: Andrey Pechkurov <37772591+puzpuzpuz@users.noreply.github.com>

lint fix

6224e88

squash: fix up links

3b98fed

Signed-off-by: Michael Dawson <mdawson@devrus.com>

squash: fix error reported only in doc-only

e180c76

Signed-off-by: Michael Dawson <mdawson@devrus.com>

mhdawson added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 26, 2021

mcollina added the blocked PRs that are blocked by other issues or PRs. label May 26, 2021

mhdawson mentioned this pull request May 31, 2021

Node.js Diagnostics WorkGroup Meeting 2021-06-02 nodejs/diagnostics#479

Closed

bl-ue approved these changes Jun 2, 2021

View reviewed changes

mcollina removed the blocked PRs that are blocked by other issues or PRs. label Jun 2, 2021

mcollina approved these changes Jun 2, 2021

View reviewed changes

bengl approved these changes Jun 2, 2021

View reviewed changes

mhdawson closed this Jun 4, 2021

danielleadams mentioned this pull request Jun 14, 2021

v16.4.0 proposal #39031

Merged

vdeturckheim mentioned this pull request Jun 18, 2021

Proposal: mark AsyncResource and AsyncLocalStorage as stable #35286

Closed

targos added dont-land-on-v12.x labels Sep 4, 2021

legendecas mentioned this pull request Jan 26, 2023

AsyncLocalStorage without Async Hooks #46265

Closed

async_hooks: stabilize part of AsyncLocalStorage by vdeturckheim · Pull Request #37675 · nodejs/node · GitHub

Uh oh!

Uh oh!

async_hooks: stabilize part of AsyncLocalStorage - #37675

async_hooks: stabilize part of AsyncLocalStorage#37675vdeturckheim wants to merge 12 commits intonodejs:masternodejs/node:masterfrom vdeturckheim:stabilize_AsyncLocalStorageCopy head branch name to clipboard

Conversation

vdeturckheim commented Mar 9, 2021

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Flarna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

vdeturckheim commented Mar 10, 2021

Uh oh!

mhdawson commented Mar 10, 2021

Uh oh!

bmeck commented Mar 10, 2021

Uh oh!

mhdawson commented Mar 10, 2021

Uh oh!

Qard commented Mar 10, 2021

Uh oh!

vdeturckheim commented Mar 11, 2021

Uh oh!

vdeturckheim commented Mar 15, 2021

Uh oh!

mhdawson commented Mar 15, 2021

Uh oh!

Qard commented Mar 18, 2021

Uh oh!

vdeturckheim commented Mar 19, 2021

Uh oh!

mhdawson commented Mar 22, 2021

Uh oh!

vdeturckheim commented Mar 25, 2021

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhdawson commented May 26, 2021

Uh oh!

mhdawson commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented May 26, 2021

Uh oh!

mhdawson commented May 26, 2021

Uh oh!

vdeturckheim commented Jun 2, 2021

Uh oh!

bl-ue left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jun 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

async_hooks: stabilize part of AsyncLocalStorage#37675
vdeturckheim wants to merge 12 commits into
nodejs:masterfrom
vdeturckheim:stabilize_AsyncLocalStorage

mhdawson commented May 26, 2021 •
edited
Loading