async_hooks: add executionAsyncResource #30959

Qard · 2019-12-14T14:59:12Z

This is a rebase of #21313

With llhttp in Node.js, the http parser reuse code now gives each reuse an AsyncResource so the concerns of the original attempt at a "current resource" API are no longer valid, unblocking it from landing now. This is basically identical to the original PR with some minor changes to appease the linter and to shuffle around some bits that moved in timers refactoring.

I'm working on this so I can update #26540 to use this form of the current resource API, eliminating a potential memory leak concern.

cc @nodejs/diagnostics @nodejs/async_hooks

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

mcollina

LGTM, we should run the benchmark and see how it goes.

src/api/callback.cc

src/async_wrap.cc

doc/api/async_hooks.md

lib/internal/async_hooks.js

Flarna

I think the calls to emitBefore() and emitAfter() in class AsyncResource needs to be also adapted to pass the current resource.

src/env.h

puzpuzpuz · 2019-12-18T15:03:59Z

@Qard
I've implemented a continuation local storage based on the executionAsyncResource() function. See #31016

Decided that you might be interested as you described this idea in #26540

doc/api/async_hooks.md

Qard · 2019-12-19T01:10:19Z

I've got the stack form almost working, but the parallel/test-heapdump-async-hooks-init-promise.js test is failing and I haven't yet figured out why. I'll continue looking into it, but if anyone else has any ideas, let me know.

Qard · 2019-12-19T01:37:14Z

Nevermind, forgot I had something commented out. It all works now. Just need to update the docs, as James requested, and it should be ready for another review. 😅

nodejs-github-bot · 2019-12-19T04:29:08Z

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

lib/async_hooks.js

doc/api/async_hooks.md

src/async_wrap.cc

Flarna · 2019-12-19T09:22:23Z

Thanks for working on this!

I left some comments but non of them are intended to block this PR as they can be addressed in a separate PR.

As far as I remember there is still one place left where an AsyncWrap is reused in node_file.cc but I'm quite sure that this can be easily fixed via a followup PR similar as the reuse in HTTP agent and parser.

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>

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>

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>

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>

Wrap reused read_wrap in a unique async resource to ensure that executionAsyncResource() is not ambiguous. PR-URL: #31972 Refs: #30959 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

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>

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>

Remove the reference from handle to the unique/wrapping resource ReusedHandle as there is meanwhile a strong reference for all async resources in place via AsyncWarp::resource_. PR-URL: nodejs#32054 Refs: nodejs#30959 Refs: nodejs#30196 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Remove the need for the destroy hook in the basic APM case. Co-authored-by: Stephen Belanger <admin@stephenbelanger.com> PR-URL: #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: #31821 Refs: #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>

This was an oversight in 9fdb6e6. Fixing this is necessary to make `executionAsyncResource()` work as expected. Refs: #30959 Fixes: #32060 PR-URL: #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>

Remove the reference from handle to the unique/wrapping resource ReusedHandle as there is meanwhile a strong reference for all async resources in place via AsyncWarp::resource_. PR-URL: #32054 Refs: #30959 Refs: #30196 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs/node#30959

Qard added lib / src Issues and PRs related to general changes in the lib or src directory. async_hooks Issues and PRs related to the async hooks subsystem. labels Dec 14, 2019

Qard requested review from watson, mcollina and AndreasMadsen December 14, 2019 14:59

Qard mentioned this pull request Dec 14, 2019

async-hooks: introduce async-storage API #26540

Closed

4 tasks

mcollina approved these changes Dec 14, 2019

View reviewed changes

Qard force-pushed the current-resource branch from b26a2ab to fab134b Compare December 14, 2019 15:20

addaleax mentioned this pull request Dec 14, 2019

executionAsyncId doesn't return expected value in context of process.on('unhandledRejection') #26794

Closed

addaleax requested changes Dec 14, 2019

View reviewed changes

src/api/callback.cc Outdated Show resolved Hide resolved

src/async_wrap.cc Outdated Show resolved Hide resolved

vdeturckheim approved these changes Dec 14, 2019

View reviewed changes

puzpuzpuz reviewed Dec 16, 2019

View reviewed changes

doc/api/async_hooks.md Outdated Show resolved Hide resolved

Flarna reviewed Dec 16, 2019

View reviewed changes

lib/internal/async_hooks.js Outdated Show resolved Hide resolved

Flarna reviewed Dec 16, 2019

View reviewed changes

puzpuzpuz mentioned this pull request Dec 18, 2019

async_hooks: add AsyncLocal API mk2 #31016

Closed

4 tasks

addaleax reviewed Dec 18, 2019

View reviewed changes

src/env.h Outdated Show resolved Hide resolved

jasnell reviewed Dec 18, 2019

View reviewed changes

doc/api/async_hooks.md Show resolved Hide resolved

Qard force-pushed the current-resource branch 2 times, most recently from a457680 to 4788019 Compare December 19, 2019 01:36

Qard force-pushed the current-resource branch 2 times, most recently from 969c029 to 793282f Compare December 19, 2019 06:17

Flarna reviewed Dec 19, 2019

View reviewed changes

lib/async_hooks.js Show resolved Hide resolved

Flarna reviewed Dec 19, 2019

View reviewed changes

doc/api/async_hooks.md Outdated Show resolved Hide resolved

Flarna reviewed Dec 19, 2019

View reviewed changes

doc/api/async_hooks.md Outdated Show resolved Hide resolved

Flarna reviewed Dec 19, 2019

View reviewed changes

src/async_wrap.cc Outdated Show resolved Hide resolved

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

kibertoad mentioned this pull request May 15, 2020

Async Local Storage for Request Context mikro-orm/mikro-orm#575

Closed

codebytere added a commit to electron/electron that referenced this pull request May 27, 2020

Fixup InternalCallbackScope patch

5bb5350

nodejs/node#30959

codebytere added a commit to electron/electron that referenced this pull request May 27, 2020

Fixup InternalCallbackScope patch

48e43e3

nodejs/node#30959

codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2020

Fixup InternalCallbackScope patch

0eefbb7

nodejs/node#30959

jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 8, 2020

Fixup InternalCallbackScope patch

c61d454

nodejs/node#30959

jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 9, 2020

Fixup InternalCallbackScope patch

ce3a7cb

nodejs/node#30959

codebytere added a commit to electron/electron that referenced this pull request Jun 15, 2020

Fixup InternalCallbackScope patch

c8f31a7

nodejs/node#30959

codebytere added a commit to electron/electron that referenced this pull request Jun 17, 2020

Fixup InternalCallbackScope patch

ed2d654

nodejs/node#30959

puzpuzpuz mentioned this pull request Jul 25, 2020

feat: add async HTTP context adonisjs/http-server#18

Merged

Flarna mentioned this pull request Aug 20, 2020

src: use symbol to store AsyncWrap resource #31745

Closed

2 tasks

Qard deleted the current-resource branch June 18, 2022 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: add executionAsyncResource #30959

async_hooks: add executionAsyncResource #30959

Qard commented Dec 14, 2019

mcollina left a comment

Flarna left a comment

puzpuzpuz commented Dec 18, 2019 •

edited

Loading

Qard commented Dec 19, 2019

Qard commented Dec 19, 2019

nodejs-github-bot commented Dec 19, 2019

Flarna commented Dec 19, 2019

async_hooks: add executionAsyncResource #30959

async_hooks: add executionAsyncResource #30959

Conversation

Qard commented Dec 14, 2019

Checklist

mcollina left a comment

Choose a reason for hiding this comment

Flarna left a comment

Choose a reason for hiding this comment

puzpuzpuz commented Dec 18, 2019 • edited Loading

Qard commented Dec 19, 2019

Qard commented Dec 19, 2019

nodejs-github-bot commented Dec 19, 2019

Flarna commented Dec 19, 2019

puzpuzpuz commented Dec 18, 2019 •

edited

Loading