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

n-api: Context for custom async operations #15189

Closed
wants to merge 4 commits into from
Closed

n-api: Context for custom async operations #15189

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
This was referenced Sep 4, 2017
Closed
Closed
@jasongin
n-api: Context for custom async operations
53de2e1 
 - 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
mhdawson
mhdawson approved these changes Sep 5, 2017
View reviewed changes
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.

@jasongin jasongin mentioned this pull request Sep 5, 2017
Closed
jasnell
jasnell reviewed Sep 7, 2017
View reviewed 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*

jasnell
jasnell approved these changes Sep 7, 2017
View reviewed changes
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

mhdawson
mhdawson approved these changes Sep 8, 2017
View reviewed changes
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

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

@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
@jasongin @mhdawson
n-api: Context for custom async operations
0c258bd 
 - 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 <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
@jasongin @addaleax
n-api: Context for custom async operations
bd8fa10 
 - 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 <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
@jasongin @jasnell
n-api: Context for custom async operations
87a3162 
 - 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 <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Sep 20, 2017
Merged
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@jasongin
n-api: Context for custom async operations
61ff9a8 
 - 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 <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
@jasongin
n-api: Context for custom async operations
edfbc5b 
 - 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 <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@jasongin @MylesBorins
n-api: Context for custom async operations
5eadd62 
 - 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 <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@mhdawson mhdawson mhdawson approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees

@mhdawson mhdawson

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate C++ AsyncHooks Embedder API with native abstraction
6 participants
@jasongin @mhdawson @BridgeAR @jasnell @MylesBorins @nodejs-github-bot