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: add executionAsyncResource #30959

Closed
wants to merge 24 commits into from
Closed

Conversation

@Qard
Copy link
Member

Qard commented Dec 14, 2019

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
@Qard Qard requested review from watson, mcollina and AndreasMadsen Dec 14, 2019
@Qard Qard mentioned this pull request Dec 14, 2019
4 of 4 tasks complete
Copy link
Member

mcollina left a comment

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

src/api/callback.cc Outdated Show resolved Hide resolved
src/async_wrap.cc Outdated Show resolved Hide resolved
doc/api/async_hooks.md Outdated Show resolved Hide resolved
lib/internal/async_hooks.js Outdated Show resolved Hide resolved
Copy link
Member

Flarna left a comment

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

@puzpuzpuz puzpuzpuz mentioned this pull request Dec 18, 2019
4 of 4 tasks complete
src/env.h Outdated Show resolved Hide resolved
@puzpuzpuz

This comment has been minimized.

Copy link
Contributor

puzpuzpuz commented Dec 18, 2019

@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

@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Dec 19, 2019

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 Qard force-pushed the Qard:current-resource branch 2 times, most recently to 4788019 Dec 19, 2019
@Qard

This comment has been minimized.

Copy link
Member Author

Qard commented Dec 19, 2019

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

This comment has been minimized.

@Qard Qard force-pushed the Qard:current-resource branch 2 times, most recently to 793282f Dec 19, 2019
lib/async_hooks.js Show resolved Hide resolved
doc/api/async_hooks.md Outdated Show resolved Hide resolved
doc/api/async_hooks.md Outdated Show resolved Hide resolved
src/async_wrap.cc Outdated Show resolved Hide resolved
@Flarna

This comment has been minimized.

Copy link
Member

Flarna commented Dec 19, 2019

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.

MylesBorins added a commit that referenced this pull request Mar 4, 2020
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>
MylesBorins added a commit that referenced this pull request Mar 4, 2020
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>
addaleax added a commit that referenced this pull request Mar 11, 2020
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>
MylesBorins added a commit that referenced this pull request Mar 11, 2020
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>
addaleax added a commit that referenced this pull request Mar 11, 2020
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>
MylesBorins added a commit that referenced this pull request Mar 11, 2020
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>
MylesBorins added a commit that referenced this pull request Mar 11, 2020
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>
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Mar 17, 2020
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>
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Mar 17, 2020
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>
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Mar 17, 2020
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>
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 2, 2020
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>
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 2, 2020
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>
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 2, 2020
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>
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 2, 2020
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>
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 2, 2020
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>
puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 2, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.