n-api: Context for custom async operations by jasongin · Pull Request #15189 · nodejs/node · GitHub
Skip to content

n-api: Context for custom async operations#15189

Closed
jasongin wants to merge 4 commits into
nodejs:masterfrom
jasongin:napi_async_context
Closed

n-api: Context for custom async operations#15189
jasongin wants to merge 4 commits into
nodejs:masterfrom
jasongin:napi_async_context

Conversation

@jasongin

@jasongin jasongin commented Sep 4, 2017

Copy link
Copy Markdown
Member
  • 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

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@mhdawson

mhdawson commented Sep 5, 2017

Copy link
Copy Markdown
Member

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

@jasnell jasnell Sep 7, 2017

Copy link
Copy Markdown
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

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

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

jasongin commented Sep 8, 2017

Copy link
Copy Markdown
Member Author

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

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

still LGTM

@jasongin

jasongin commented Sep 9, 2017

Copy link
Copy Markdown
Member Author

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

BridgeAR commented Sep 9, 2017

Copy link
Copy Markdown
Member

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

@BridgeAR

BridgeAR commented Sep 11, 2017

Copy link
Copy Markdown
Member

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

@jasongin

jasongin commented Sep 11, 2017

Copy link
Copy Markdown
Member Author

I pushed a commit to fix the lint issues.

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

@mhdawson

mhdawson commented Sep 14, 2017

Copy link
Copy Markdown
Member

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

@mhdawson

mhdawson commented Sep 14, 2017

Copy link
Copy Markdown
Member

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

@mhdawson mhdawson self-assigned this Sep 14, 2017
@mhdawson

mhdawson commented Sep 14, 2017

Copy link
Copy Markdown
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

mhdawson commented Sep 14, 2017

Copy link
Copy Markdown
Member

#14697 landed, going to land this one.

@mhdawson

mhdawson commented Sep 14, 2017

Copy link
Copy Markdown
Member

@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
@addaleax addaleax Awaiting requested review from addaleax
+1 more reviewer
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

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