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

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

bmeck
bmeck approved these changes Mar 9, 2021
Copy link
Member

@mhdawson mhdawson left a comment

LGTM

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

@mcollina mcollina left a comment

lgtm

cjihrig
cjihrig approved these changes Mar 9, 2021
@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
@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

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

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

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 4 commits May 25, 2021
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 force-pushed the 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

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

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

@mcollina mcollina left a comment

lgtm

bengl
bengl approved these changes Jun 2, 2021
mhdawson added a commit that referenced this issue 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 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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