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

n-api: Context for custom async operations #15189

Closed
wants to merge 4 commits into from

Conversation

jasongin
Copy link
Member

@jasongin jasongin commented Sep 4, 2017

  • Add napi_async_context opaque pointer type. (If needed, we could later add APIs for getting the async IDs out of this context.)
  • Add napi_async_init() and napi_async_destroy() APIs.
  • Add async_context parameter to napi_make_callback().
  • Add code and checks to test_make_callback to validate async context APIs by checking async hooks are called with correct context.
  • Update API documentation.

Fixes: #13254

See also the related PR #14697 in which napi_create_async_work() is updated to automatically track async context. That PR together with this one make up the complete story for async context tracking with N-API.

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
Affected core subsystem(s)

n-api

@jasongin jasongin added the node-api Issues and PRs related to the Node-API. label Sep 4, 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 Sep 4, 2017
 - Add napi_async_context opaque pointer type.
   (If needed, we could later add APIs for getting the async IDs
   out of this context.)
 - Add napi_async_init() and napi_async_destroy() APIs.
 - Add async_context parameter to napi_make_callback().
 - Add code and checks to test_make_callback to validate async context
   APIs by checking async hooks are called with correct context.
 - Update API documentation.

Fixes: nodejs#13254
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM.

@mhdawson
Copy link
Member

mhdawson commented Sep 5, 2017

Last N-API meeting we agreed to try to land all of the breaking changes together which I believe includes this one, as well so we should co-ordinate landing with the other breaking PRs. The goal is to have one final set of breaking changes.

doc/api/n-api.md Outdated
operation (when there is no other script on the stack). It is a fairly simple
wrapper around `node::MakeCallback`.

Note it is NOT necessary to use `napi_make_callback` from within a
Copy link
Member

Choose a reason for hiding this comment

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

s/NOT/*not*

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

@jasongin
Copy link
Member Author

jasongin commented Sep 8, 2017

As discussed for #15108, I need to change the string parameter of napi_async_init() to a napi_value.

I also suggested a corresponding change to the string parameter of napi_create_async_work() at https://github.com/nodejs/node/pull/14697/files#r137702655

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

still LGTM

@jasongin
Copy link
Member Author

jasongin commented Sep 9, 2017

Update:

  • Changed the name parameter from char* to napi_value
  • That required adding a new public overload for node::EmitAsyncInit()
  • Made the resource object parameter optional, to match the documentation.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

@BridgeAR
Copy link
Member

There seem to be some linter errors https://ci.nodejs.org/job/node-test-commit/12275/

@jasongin
Copy link
Member Author

I pushed a commit to fix the lint issues.

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

@mhdawson
Copy link
Member

CI failure on ubuntu16 was a infra failure, not related to this PR.

@mhdawson
Copy link
Member

Arm failure was also a infra failure, not related to this PR.

@mhdawson mhdawson self-assigned this Sep 14, 2017
@mhdawson
Copy link
Member

Looks like all of the breaking changes may be ready to go today (just watching #14697). Once it lands we'll land this and the other remaining breaking changes.

@mhdawson
Copy link
Member

#14697 landed, going to land this one.

@mhdawson
Copy link
Member

Landed as 0c258bd

@mhdawson mhdawson closed this Sep 14, 2017
mhdawson pushed a commit that referenced this pull request Sep 14, 2017
 - Add napi_async_context opaque pointer type.
   (If needed, we could later add APIs for getting the async IDs
   out of this context.)
 - Add napi_async_init() and napi_async_destroy() APIs.
 - Add async_context parameter to napi_make_callback().
 - Add code and checks to test_make_callback to validate async context
   APIs by checking async hooks are called with correct context.
 - Update API documentation.

PR-URL: #15189
Fixes: #13254
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
 - Add napi_async_context opaque pointer type.
   (If needed, we could later add APIs for getting the async IDs
   out of this context.)
 - Add napi_async_init() and napi_async_destroy() APIs.
 - Add async_context parameter to napi_make_callback().
 - Add code and checks to test_make_callback to validate async context
   APIs by checking async hooks are called with correct context.
 - Update API documentation.

PR-URL: nodejs/node#15189
Fixes: nodejs/node#13254
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
 - Add napi_async_context opaque pointer type.
   (If needed, we could later add APIs for getting the async IDs
   out of this context.)
 - Add napi_async_init() and napi_async_destroy() APIs.
 - Add async_context parameter to napi_make_callback().
 - Add code and checks to test_make_callback to validate async context
   APIs by checking async hooks are called with correct context.
 - Update API documentation.

PR-URL: #15189
Fixes: #13254
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
 - Add napi_async_context opaque pointer type.
   (If needed, we could later add APIs for getting the async IDs
   out of this context.)
 - Add napi_async_init() and napi_async_destroy() APIs.
 - Add async_context parameter to napi_make_callback().
 - Add code and checks to test_make_callback to validate async context
   APIs by checking async hooks are called with correct context.
 - Update API documentation.

PR-URL: nodejs/node#15189
Fixes: nodejs/node#13254
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
 - Add napi_async_context opaque pointer type.
   (If needed, we could later add APIs for getting the async IDs
   out of this context.)
 - Add napi_async_init() and napi_async_destroy() APIs.
 - Add async_context parameter to napi_make_callback().
 - Add code and checks to test_make_callback to validate async context
   APIs by checking async hooks are called with correct context.
 - Update API documentation.

PR-URL: nodejs#15189
Fixes: nodejs#13254
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
 - Add napi_async_context opaque pointer type.
   (If needed, we could later add APIs for getting the async IDs
   out of this context.)
 - Add napi_async_init() and napi_async_destroy() APIs.
 - Add async_context parameter to napi_make_callback().
 - Add code and checks to test_make_callback to validate async context
   APIs by checking async hooks are called with correct context.
 - Update API documentation.

Backport-PR-URL: #19447
PR-URL: #15189
Fixes: #13254
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate C++ AsyncHooks Embedder API with native abstraction
6 participants