[v12.x backport] async_hooks: add executionAsyncResource by puzpuzpuz · Pull Request #32131 · nodejs/node · GitHub

Backports executionAsyncResource into v12.x. I've cherry picked the following commits/PRs:

Had to resolve only a single (minor) conflict in lib/internal/async_hooks.js -> emitBeforeScript.

cc @Qard @vdeturckheim @addaleax @Flarna

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

It looks like CI failed due to timeout in compilation phase. I've just tried a fresh compilation and a test run on this branch and they went fine on my machine.

Could someone restart Travis build?

CI: https://ci.nodejs.org/job/node-test-pull-request/29605/

Could you please add links to the originial PRs in the PR description?

Could you please add links to the originial PRs in the PR description?

Sure thing. I've updated the description.

Failed tests look weird and unrelated with this PR. Are there known flaky tests in v12.x-staging or I'm wrong and those failures are relevant?

CI: https://ci.nodejs.org/job/node-test-pull-request/29609/

As far as I can see, failed tests (at least, some of them) are related with #32080. Namely, this regular expression has to be fixed now, as CHANGELOG.md has changed in master:
https://github.com/nodejs/node/blob/v12.x-staging/tools/doc/versions.js#L58

Should I cherry pick that PR as well or it's better to create a separate PR to backport those changes?

Also, it seems that test/parallel/test-dgram-connect* and test/parallel/test-dgram-send* tests are failing on OS X 10.15 (and only there), but I'm not sure what to do with those. They may be flaky, or there may be a fix that wasn't backported yet.

I've seen the test-dgram errors on other PRs against the master branch.

I've seen the test-dgram errors on other PRs against the master branch.

I don't think #31936 has landed on v12.x-staging yet.

I've created #32146 to deal with test failures discussed above. We have to wait for it to land first before we get a green build here.

I just did a rebase, as #32146 was merged today. Hopefully, CI builds should be able to pass successfully now.

Could someone trigger a CI build on this one?

Rebased and resolved a minor conflict (appeared after 0b3bee5).

I'm seeing the following error in make -j4 test output after the rebase:

Running JS linter...
/home/puzpuzpuz/projects/node/tools/doc/allhtml.js:87
  if (!ids.has(match[1])) throw new Error(`link not found: ${match[1]}`);
                          ^

Error: link not found: repl_reverse_i_search
    at Object.<anonymous> (/home/puzpuzpuz/projects/node/tools/doc/allhtml.js:87:33)
    at Module._compile (internal/modules/cjs/loader.js:1158:30)
    ...
    at internal/main/run_main_module.js:18:47
Makefile:754: recipe for target 'out/doc/api/all.html' failed

Looks like the issue is not related with this PR and located in v12.x-staging (see #32280 (comment)).

Update. Looks like the problem is the following. #31256 was backported, but it included this this change which depends on #31006 (and this PR wasn't backported). I've created #32282 to deal with that.

lgtm

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2310/

Overall this needs to be watched carefully as it might introduce regressions.

Thanks for running a CITGM build. The changes in this PR may introduce some regressions, indeed.

I can see 20 failures in this build, but I'm not sure if they're related with this PR or they're false positives. I can see that other CITGM builds also fail with similar (if not the same) errors.

Update. I went thought failed tests output and didn't find anything suspicious: they seem to be failing because of some timeouts and flaky tests.

@Trott @MylesBorins could you take a look as well?

CITGM results look good to me.

lgtm

Rebased to the latest v12.x-staging

CI: https://ci.nodejs.org/job/node-test-pull-request/30499/

Not sure if backporting to LTS should be delayed a little as breaking API changes are in pipeline (#31950).

@Flarna

Not sure if backporting to LTS should be delayed a little as breaking API changes are in pipeline (#31950).

Those changes are about ALS, while this PR backports executionAsyncResounce only.

Yep. That change should not impact this. Also, I would argue to not consider it breaking as ALS is experimental and still very new. I think we should consider those changes not major and therefore backportable.

yes, sorry. mixed this up.

CI: https://ci.nodejs.org/job/node-test-pull-request/30514/

CI: https://ci.nodejs.org/job/node-test-pull-request/30515/

Remove the need for the destroy hook in the basic APM case. Co-authored-by: Stephen Belanger <admin@stephenbelanger.com> PR-URL: nodejs#30959 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Ensure that resource returned by executionAsyncResource() in before and after hook matches that resource causing this before/after calls. PR-URL: nodejs#31821 Refs: nodejs#30959 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>

PR-URL: nodejs#31944 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

This was an oversight in 9fdb6e6. Fixing this is necessary to make `executionAsyncResource()` work as expected. Refs: nodejs#30959 Fixes: nodejs#32060 PR-URL: nodejs#32063 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

CI: https://ci.nodejs.org/job/node-test-pull-request/30711/

CI: https://ci.nodejs.org/job/node-test-pull-request/30716/

CI: https://ci.nodejs.org/job/node-test-pull-request/30723/

Landed on my release preparation branch: https://github.com/targos/node/commits/prepare-minor

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Mar 6, 2020

puzpuzpuz mentioned this pull request Mar 6, 2020

async-hooks: introduce async-storage API #26540

Closed

4 tasks

Qard approved these changes Mar 7, 2020

View reviewed changes

vdeturckheim approved these changes Mar 7, 2020

View reviewed changes

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 7, 2020

Flarna approved these changes Mar 7, 2020

View reviewed changes

richardlau mentioned this pull request Mar 8, 2020

doc: remove em dashes #32080

Merged

3 tasks

puzpuzpuz mentioned this pull request Mar 8, 2020

[v12.x backport] doc: remove em dashes #32146

Closed

3 tasks

puzpuzpuz changed the title [v12.x] async_hooks: backport executionAsyncResource to v12.x [v12.x backport] async_hooks: add executionAsyncResource Mar 8, 2020

puzpuzpuz force-pushed the execution-async-resource-backport branch from 21223ff to 838907d Compare March 14, 2020 19:49

Qard approved these changes Mar 14, 2020

View reviewed changes

puzpuzpuz force-pushed the execution-async-resource-backport branch from 838907d to 96b06a9 Compare March 15, 2020 12:09

This was referenced Mar 15, 2020

doc: add AsyncResource + Worker pool example #31601

Closed

async_hooks: introduce mandatory store argument in AsyncLocalStorage run methods #31930

Closed

codebytere force-pushed the v12.x-staging branch from 2047cca to 0606e1d Compare March 17, 2020 02:49

puzpuzpuz force-pushed the execution-async-resource-backport branch from 96b06a9 to 8d6f6fe Compare March 17, 2020 04:40

mcollina approved these changes Mar 17, 2020

View reviewed changes

mcollina approved these changes Mar 22, 2020

View reviewed changes

codebytere force-pushed the v12.x-staging branch from a01fb4a to e5dc2fb Compare March 23, 2020 16:01

codebytere force-pushed the v12.x-staging branch 2 times, most recently from 63a03d2 to d577190 Compare March 31, 2020 23:57

puzpuzpuz force-pushed the execution-async-resource-backport branch from 8d6f6fe to d8de7e5 Compare April 2, 2020 13:17

Qard approved these changes Apr 6, 2020

View reviewed changes

mcollina and others added 4 commits April 14, 2020 10:41

src: move InternalCallbackScope to StartExecution

0e12a43

PR-URL: nodejs#31944 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

puzpuzpuz force-pushed the execution-async-resource-backport branch from d8de7e5 to fc50e29 Compare April 14, 2020 07:41

Flarna mentioned this pull request Apr 22, 2020

http, async_hooks: remove unneeded reference to wrapping resource #32054

Closed

2 tasks

targos closed this Apr 25, 2020

puzpuzpuz deleted the execution-async-resource-backport branch April 26, 2020 06:56

[v12.x backport] async_hooks: add executionAsyncResource by puzpuzpuz · Pull Request #32131 · nodejs/node · GitHub

Uh oh!

Uh oh!

[v12.x backport] async_hooks: add executionAsyncResource - #32131

[v12.x backport] async_hooks: add executionAsyncResource#32131puzpuzpuz wants to merge 4 commits intonodejs:v12.x-stagingnodejs/node:v12.x-stagingfrom puzpuzpuz:execution-async-resource-backportpuzpuzpuz/node:execution-async-resource-backportCopy head branch name to clipboard

Conversation

puzpuzpuz commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

puzpuzpuz commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 7, 2020

Uh oh!

Flarna commented Mar 7, 2020

Uh oh!

puzpuzpuz commented Mar 7, 2020

Uh oh!

puzpuzpuz commented Mar 7, 2020

Uh oh!

nodejs-github-bot commented Mar 7, 2020

Uh oh!

puzpuzpuz commented Mar 7, 2020

Uh oh!

targos commented Mar 7, 2020

Uh oh!

richardlau commented Mar 7, 2020

Uh oh!

puzpuzpuz commented Mar 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puzpuzpuz commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puzpuzpuz commented Mar 15, 2020

Uh oh!

puzpuzpuz commented Mar 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

puzpuzpuz commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Mar 17, 2020

Uh oh!

Trott commented Mar 22, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

puzpuzpuz commented Apr 2, 2020

Uh oh!

nodejs-github-bot commented Apr 6, 2020

Uh oh!

Flarna commented Apr 6, 2020

Uh oh!

puzpuzpuz commented Apr 6, 2020

Uh oh!

Qard commented Apr 6, 2020

Uh oh!

Flarna commented Apr 6, 2020

Uh oh!

nodejs-github-bot commented Apr 6, 2020

Uh oh!

nodejs-github-bot commented Apr 6, 2020

Uh oh!

nodejs-github-bot commented Apr 14, 2020

Uh oh!

nodejs-github-bot commented Apr 14, 2020

Uh oh!

nodejs-github-bot commented Apr 14, 2020

Uh oh!

targos commented Apr 25, 2020

Uh oh!

Reviewers

Assignees

Labels

[v12.x backport] async_hooks: add executionAsyncResource#32131
puzpuzpuz wants to merge 4 commits into
nodejs:v12.x-stagingfrom
puzpuzpuz:execution-async-resource-backport

puzpuzpuz commented Mar 6, 2020 •
edited
Loading

puzpuzpuz commented Mar 7, 2020 •
edited
Loading

puzpuzpuz commented Mar 8, 2020 •
edited
Loading

puzpuzpuz commented Mar 14, 2020 •
edited
Loading

puzpuzpuz commented Mar 15, 2020 •
edited
Loading

puzpuzpuz commented Mar 17, 2020 •
edited
Loading