Sitelet https://web.archive.org/web/20210813062740/https://github.com/nodejs/node/pull/37675
Skip to content

/ node

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

Jump to bottom

async_hooks: stabilize part of AsyncLocalStorage #37675

Closed
wants to merge 12 commits into from
Closed

async_hooks: stabilize part of AsyncLocalStorage #37675

wants to merge 12 commits into from
+606 −560

Conversation

@vdeturckheim
Copy link
Member

@vdeturckheim vdeturckheim commented Mar 9, 2021

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
@vdeturckheim vdeturckheim requested review from puzpuzpuz and Qard Mar 9, 2021
@bmeck
bmeck approved these changes Mar 9, 2021
View changes
@puzpuzpuz
puzpuzpuz approved these changes Mar 9, 2021
View changes
@vdeturckheim vdeturckheim mentioned this pull request Mar 9, 2021
Open
@mhdawson
mhdawson approved these changes Mar 9, 2021
View changes
Copy link
Member

@mhdawson mhdawson left a comment

LGTM
@Flarna
Flarna reviewed Mar 9, 2021
View changes
Copy link
Member

@Flarna Flarna left a comment

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.
doc/api/async_hooks.md Outdated Show resolved Hide resolved
@Qard
Qard approved these changes Mar 9, 2021
View changes
@mcollina
mcollina approved these changes Mar 9, 2021
View changes
Copy link
Member

@mcollina mcollina left a comment

lgtm
@cjihrig
cjihrig approved these changes Mar 9, 2021
View changes
@vdeturckheim
Copy link
Member Author

@vdeturckheim vdeturckheim commented Mar 10, 2021

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?
@Flarna
Flarna approved these changes Mar 10, 2021
View changes
@mhdawson
Copy link
Member

@mhdawson mhdawson commented Mar 10, 2021

@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
@bmeck
Copy link
Member

@bmeck bmeck commented Mar 10, 2021

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.
@mhdawson
Copy link
Member

@mhdawson mhdawson commented Mar 10, 2021

I was not suggesting changing the import specifier, just the organization of the docs.
@Qard
Copy link
Member

@Qard Qard commented Mar 10, 2021

👍 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?
@vdeturckheim
Copy link
Member Author

@vdeturckheim vdeturckheim commented Mar 11, 2021

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

@mhdawson good idea!

I'll update the PR
@aymen94
aymen94 approved these changes Mar 11, 2021
View changes
@vdeturckheim
Copy link
Member Author

@vdeturckheim vdeturckheim commented Mar 15, 2021

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

@mhdawson mhdawson commented Mar 15, 2021

@vdeturckheim that sounds reasonable to me.
@legendecas
legendecas approved these changes Mar 17, 2021
View changes
@Qard
Copy link
Member

@Qard Qard commented Mar 18, 2021

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.
@vdeturckheim vdeturckheim marked this pull request as draft Mar 19, 2021
@vdeturckheim
Copy link
Member Author

@vdeturckheim vdeturckheim commented Mar 19, 2021

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)
@mhdawson
Copy link
Member

@mhdawson mhdawson commented Mar 22, 2021

@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.
@vdeturckheim
Copy link
Member Author

@vdeturckheim vdeturckheim commented Mar 25, 2021

@mhdawson agreed. I updated a bit. It should start to look good now.
@Qard
Qard reviewed Mar 25, 2021
View changes
Copy link
Member

@Qard Qard left a comment

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.
doc/api/async_context.md Outdated Show resolved Hide resolved
doc/api/async_context.md Outdated Show resolved Hide resolved
vdeturckheim and others added 6 commits Apr 28, 2021
@vdeturckheim @puzpuzpuz @mhdawson
Update doc/api/async_context.md
5d369ad 
Co-authored-by: Andrey Pechkurov <37772591+puzpuzpuz@users.noreply.github.com>
@vdeturckheim @Qard @mhdawson
Update doc/api/async_context.md
e90e356 
Co-authored-by: Stephen Belanger <admin@stephenbelanger.com>
@vdeturckheim @targos
@puzpuzpuz @mhdawson
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>
@vdeturckheim @mhdawson
lint fix
6224e88
@mhdawson
squash: fix up links
3b98fed 
Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson
squash: fix error reported only in doc-only
e180c76 
Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson mhdawson force-pushed the vdeturckheim:stabilize_AsyncLocalStorage branch from c0b52ba to e180c76 May 26, 2021
@mhdawson
Copy link
Member

@mhdawson mhdawson commented May 26, 2021

Doc only, so full CI not required.
@mhdawson
Copy link
Member

@mhdawson mhdawson commented May 26, 2021 •
edited

@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).
@mcollina
Copy link
Member

@mcollina mcollina commented May 26, 2021

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

@mhdawson mhdawson commented May 26, 2021

@mcollina thanks for the heads up.
@mhdawson mhdawson mentioned this pull request May 31, 2021
Closed
@vdeturckheim
Copy link
Member Author

@vdeturckheim vdeturckheim commented Jun 2, 2021

@mhdawson #38821 has landed :)
@bl-ue
bl-ue approved these changes Jun 2, 2021
View changes
Copy link
Contributor

@bl-ue bl-ue left a comment

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.
@mcollina mcollina removed the blocked label Jun 2, 2021
@mcollina
mcollina approved these changes Jun 2, 2021
View changes
Copy link
Member

@mcollina mcollina left a comment

lgtm
@bengl
bengl approved these changes Jun 2, 2021
View changes
mhdawson added a commit that referenced this pull request Jun 4, 2021
@vdeturckheim @mhdawson
async_hooks: stabilize part of AsyncLocalStorage
7612d82 
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>
@mhdawson
Copy link
Member

@mhdawson mhdawson commented Jun 4, 2021

Landed in 7612d82

@vdeturckheim thanks for all your work on this.
@mhdawson mhdawson closed this Jun 4, 2021
targos added a commit that referenced this pull request Jun 11, 2021
@vdeturckheim @targos
async_hooks: stabilize part of AsyncLocalStorage
8115e6e 
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>
@danielleadams danielleadams mentioned this pull request Jun 14, 2021
Merged
@vdeturckheim vdeturckheim mentioned this pull request Jun 18, 2021
Closed
danielleadams added a commit that referenced this pull request Jun 22, 2021
@danielleadams
2021-06-22, Version 16.4.0 (Current)
6e6f9d0 
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
danielleadams added a commit that referenced this pull request Jun 22, 2021
@danielleadams
2021-06-22, Version 16.4.0 (Current)
8a19b2b 
PR-URL: #39031

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
danielleadams added a commit that referenced this pull request Jun 22, 2021
@danielleadams
2021-06-22, Version 16.4.0 (Current)
c537f5f 
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
danielleadams added a commit that referenced this pull request Jun 23, 2021
@danielleadams
2021-06-23, Version 16.4.0 (Current)
4b2085f 
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
danielleadams added a commit that referenced this pull request Jun 23, 2021
@danielleadams
2021-06-23, Version 16.4.0 (Current)
84d6ce9 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qard Qard

@targos targos

@legendecas legendecas

@puzpuzpuz puzpuzpuz

@mcollina mcollina

@bengl bengl

@bmeck bmeck

@addaleax addaleax

@cjihrig cjihrig

@mhdawson mhdawson

@aymen94 aymen94

@Flarna Flarna

@bl-ue bl-ue

Assignees
No one assigned
Labels
async_hooks author ready diag-agenda doc semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet