Skip to content

Conversation

@addaleax
Copy link
Member

  • 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)

@addaleaxaddaleax 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-botnodejs-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
@jasongin
Copy link
Member

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
MemberAuthor

@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

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

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
MemberAuthor

@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!

@addaleaxaddaleaxforce-pushed the foo branch 2 times, most recently from 4dceb50 to c5cc1cfCompareAugust 8, 2017 21:37
@addaleaxaddaleaxforce-pushed the foo branch 3 times, most recently from 65dfc41 to b7f6ca7CompareAugust 14, 2017 22:37
@addaleax
Copy link
MemberAuthor

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/

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
MemberAuthor

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
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
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
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
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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
Backport-PR-URL: #19447 PR-URL: #14697 Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Apr 16, 2018
@parro-itparro-it mentioned this pull request May 11, 2018
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addonsIssues and PRs related to native addons.async_hooksIssues and PRs related to the async hooks subsystem.c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.node-apiIssues and PRs related to the Node-API.semver-minorPRs 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.

10 participants

@addaleax@jasongin@refack@TimothyGu@trevnorris@jasnell@thefourtheye@tniessen@mhdawson@nodejs-github-bot