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 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 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
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, 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 Flarna left a comment

Choose a reason for hiding this comment

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

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

src/env.h Outdated Show resolved Hide resolved
@puzpuzpuz
Copy link
Member

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
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 current-resource branch 2 times, most recently from a457680 to 4788019 Compare December 19, 2019 01:36
@Qard
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
Copy link
Collaborator

@Qard Qard force-pushed the current-resource branch 2 times, most recently from 969c029 to 793282f Compare December 19, 2019 06:17
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
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.

puzpuzpuz pushed a commit to puzpuzpuz/node that referenced this pull request Apr 14, 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 pushed a commit to puzpuzpuz/node that referenced this pull request Apr 14, 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 pushed a commit to puzpuzpuz/node that referenced this pull request Apr 14, 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 pushed a commit to puzpuzpuz/node that referenced this pull request Apr 14, 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 pushed a commit to puzpuzpuz/node that referenced this pull request Apr 14, 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 pushed a commit to puzpuzpuz/node that referenced this pull request Apr 14, 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>
targos pushed a commit that referenced this pull request Apr 22, 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>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 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>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 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>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 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>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 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: 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>
targos pushed a commit that referenced this pull request Apr 28, 2020
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>
targos pushed a commit that referenced this pull request Apr 28, 2020
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>
targos pushed a commit that referenced this pull request Apr 28, 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>
targos pushed a commit that referenced this pull request Apr 28, 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>
codebytere added a commit to electron/electron that referenced this pull request May 27, 2020
codebytere added a commit to electron/electron that referenced this pull request May 27, 2020
codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2020
jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 8, 2020
jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 9, 2020
codebytere added a commit to electron/electron that referenced this pull request Jun 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Jun 17, 2020
@Qard Qard deleted the current-resource branch June 18, 2022 20:25
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. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. notable-change PRs with changes that should be highlighted in changelogs. 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.