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: 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

Conversation

vdeturckheim
Copy link
Member

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

@nodejs-github-bot 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
@mhdawson mhdawson added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 9, 2021
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@Flarna Flarna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@vdeturckheim
Copy link
Member Author

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?

@mhdawson
Copy link
Member

@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 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

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

@Qard
Copy link
Member

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

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

@mhdawson good idea!

I'll update the PR

@vdeturckheim
Copy link
Member Author

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

@vdeturckheim that sounds reasonable to me.

@Qard
Copy link
Member

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 March 19, 2021 14:02
@vdeturckheim
Copy link
Member Author

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

@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

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

Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 4 commits May 25, 2021 19:57
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>
Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson mhdawson added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 26, 2021
@mhdawson
Copy link
Member

Doc only, so full CI not required.

@mhdawson
Copy link
Member

mhdawson commented May 26, 2021

@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

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

@mcollina mcollina added the blocked PRs that are blocked by other issues or PRs. label May 26, 2021
@mhdawson
Copy link
Member

@mcollina thanks for the heads up.

@vdeturckheim
Copy link
Member Author

@mhdawson #38821 has landed :)

Copy link
Contributor

@bl-ue bl-ue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 PRs that are blocked by other issues or PRs. label Jun 2, 2021
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

mhdawson pushed a commit that referenced this pull request Jun 4, 2021
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 commented Jun 4, 2021

Landed in 7612d82

@vdeturckheim thanks for all your work on this.

@mhdawson mhdawson closed this Jun 4, 2021
targos pushed a commit that referenced this pull request Jun 11, 2021
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
danielleadams added a commit that referenced this pull request Jun 22, 2021
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
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
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
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
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
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. doc Issues and PRs related to the documentations. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.