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.

Sign up for GitHub

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

Jump to bottom

src,async_hooks,n-api: refactor async callback handling #14697

Closed
wants to merge 8 commits into from
Closed

src,async_hooks,n-api: refactor async callback handling #14697

wants to merge 8 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 8, 2017

  • Refactor how asynchronous code is executed from within Node
  • Merge the two almost-but-not-quite identical MakeCallback() implementations
  • Provide a public CallbackScope class for embedders in order to enable MakeCallback()-like behaviour without tying that to calling a JS function
  • Enable combining N-API async work with async-hooks
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

/cc @nodejs/async_hooks
/cc @nodejs/n-api (Who may or may not only want to review the N-API commit; this is a breaking N-API change)

@addaleax addaleax added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap 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. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 8, 2017
@nodejs-github-bot nodejs-github-bot added 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. labels Aug 8, 2017
@addaleax
Copy link
Member Author

addaleax commented Aug 8, 2017 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-commit/11632/
CI: https://ci.nodejs.org/job/node-test-commit/11633/
CI: https://ci.nodejs.org/job/node-test-commit/11634/
CI: https://ci.nodejs.org/job/node-test-commit/11635/
CI: https://ci.nodejs.org/job/node-test-commit/11636/
CI: https://ci.nodejs.org/job/node-test-commit/11637/

@jasongin
Copy link
Member

jasongin commented Aug 8, 2017

N-API parts look good.

I'm still planning to add an async context parameter to napi_make_callback() to support other async scenarios that don't use the N-API async work APIs.

If I'm understanding correctly, the change in this PR means that when the N-API async work APIs are used, during the napi_async_complete_callback invocation the current async context is already correct. Usually that callback will then invoke napi_make_callback(). So the additional async context parameter to napi_make_callback() can be optional, and the current async context used if another context is not specified.

@addaleax
Copy link
Member Author

addaleax commented Aug 8, 2017

@jasongin This PR would mean that napi_make_callback would be unnecessary for most completion C callbacks; N-API users who use the async work API wouldn’t need to bother with any of the async stuff and could just use a plain function call.

@jasongin
Copy link
Member

jasongin commented Aug 8, 2017

Oh, so the async work callback should just call napi_call_function() instead? OK, that's simpler.

Then the only need for napi_make_callback() (updated with async context parameter) will be for async code that doesn't use the async work API.

@jasongin
Copy link
Member

jasongin commented Aug 8, 2017

In that case should test/addons-napi/test_async/test_async.cc be updated to call napi_call_function() instead of napi_make_callback() (in 2 places)?

@addaleax
Copy link
Member Author

addaleax commented Aug 8, 2017

@jasongin Exactly 👍

In that case should test/addons-napi/test_async/test_async.cc be updated to call napi_call_function() instead of napi_make_callback() (in 2 places)?

Right. Done!

@addaleax addaleax mentioned this pull request Aug 10, 2017
Closed
3 tasks
@addaleax
Copy link
Member Author

Rebased … maybe somebody of @AndreasMadsen @trevnorris @refack @TimothyGu @tniessen @bnoordhuis @jasnell could be reviewer (if only for the yet-unreviewed non-N-API part)?

CI: https://ci.nodejs.org/job/node-test-commit/11769/

thefourtheye
thefourtheye reviewed Aug 14, 2017
View reviewed changes
doc/api/n-api.md Outdated
napi_async_execute_callback execute,
napi_async_complete_callback complete,
void* data,
napi_async_work* result);
```

- `[in] env`: The environment that the API is invoked under.
- `[in] async_resource_name`: An identifier for the kind of resource that is
being provided for diagnostic information expose by the `async_hooks` API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: exposed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, fixed.

Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@addaleax
test: add regression test for 5691
2defa68 
With `CallbackScope`, this has become possible to do properly.

Fixes: nodejs/node#5691
PR-URL: nodejs/node#14697
Reviewed-By: James M Snell <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@addaleax
src: move DomainEnter,DomainExit to node.cc
c015915 
PR-URL: nodejs/node#14697
Reviewed-By: Trevor Norris <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@jasongin
n-api: change async resource name to napi_value
e6590ff 
PR-URL: nodejs/node#14697
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
Closed
2 tasks
@aruneshchandra aruneshchandra mentioned this pull request Dec 5, 2017
Closed
@mhdawson mhdawson mentioned this pull request Mar 5, 2018
Closed
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
@addaleax
n-api: use AsyncResource for Work tracking
9c68371 
Enable combining N-API async work with async-hooks.

PR-URL: nodejs#14697
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
@jasongin
n-api: change async resource name to napi_value
14dc0f8 
PR-URL: nodejs#14697
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@addaleax @MylesBorins
n-api: use AsyncResource for Work tracking
7588eea 
Enable combining N-API async work with async-hooks.

Backport-PR-URL: #19447
PR-URL: #14697
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@jasongin @MylesBorins
n-api: change async resource name to napi_value
156a8b6 
Backport-PR-URL: #19447
PR-URL: #14697
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
@parro-it parro-it mentioned this pull request May 11, 2018
Merged
2 tasks
@simonbuchan simonbuchan mentioned this pull request Feb 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasongin jasongin jasongin approved these changes

@tniessen tniessen tniessen approved these changes

@TimothyGu TimothyGu TimothyGu left review comments

@thefourtheye thefourtheye thefourtheye left review comments

@jasnell jasnell jasnell approved these changes

@trevnorris trevnorris trevnorris requested changes

@mhdawson mhdawson mhdawson approved these changes

@refack refack refack approved these changes

@AndreasMadsen AndreasMadsen Awaiting requested review from AndreasMadsen

Assignees

@jasnell jasnell

Labels
addons Issues and PRs related to native addons. 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. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@addaleax @jasongin @refack @TimothyGu @trevnorris @jasnell @thefourtheye @tniessen @mhdawson @nodejs-github-bot