n-api: implement async helper methods#12250

boingoing · 2017-04-06T02:32:54Z

Based on the async methods we had in abi-stable-node before the napi
feature landed in node/master. Changed this set of APIs to handle
error cases and removed a lot of the extra methods we had for setting
all the pieces of napi_work opting instead to pass all of those as
arguments to napi_create_async_work as none of those parameters are
optional except for the complete callback, anyway.

Renamed the napi_work struct to napi_async_work and replace the
struct itself with a class which can better encapsulate the object
lifetime and uv_work_t that we're trying to wrap anyway.

Added a napi_async_callback type for the async helper callbacks
instead of taking raw function pointers and make this callback take a
napi_env parameter as well as the void* data it was already taking.

Call the complete handler for the async work item with a napi_status
code translated from the uvlib error code.

The execute callback is required for napi_create_async_work, though
complete callback is still optional.

Also added some async unit tests for addons-napi based on the
addons/async_hello_world test.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

n-api

boingoing · 2017-04-06T02:58:21Z

@jasongin @digitalinfinity @mhdawson @sampsongao
Please take a look - this is the port of the async methods.

kkoopa · 2017-04-06T07:04:29Z

Would it be reasonable to use unique_ptr instead of handing out raw pointers in the Work class? Is there an ABI issue?

bnoordhuis · 2017-04-06T10:01:19Z

Why would you do this in napi instead of nan? Seems like feature creep.

addaleax · 2017-04-06T12:50:02Z

@bnoordhuis See also #11975 (comment). I am still not 100 % sure about this either…

mhdawson

LGTM

mhdawson · 2017-04-06T20:03:12Z

@bnoordhuis there was also a comment from @kkoopa on this issue: nodejs/abi-stable-node#204

In response to discussion about removing from the original PR (which we did temporarily, and are now adding back in):

 Also, if this specific issue is a concern, we could consider removing the AsyncWorker class from the C++ wrapper library. Its implementation is a fairly trivial composition of other N-API wrappers (Napi::ObjectReference and Napi::FunctionReference) with the libuv async work callbacks.

No, no. It is nowhere near a trivial composition for the average person developing a native addon. The primary reason for writing a native addon is necessity. Many addons are cobbled together from looking at existing addons, without much understanding of what goes on or what errors there are. These then trickle up the dependency chain and break the ecosystem. There is a definite need of cookie-cutter functionality for common use cases, which ensures that things are done right and reduce boilerplate. Besides ABI stability, I do not consider it good that writing a native addon correctly requires intimate knowledge of the node.js API, the libuv API as well as how they interact. There should be one node.js API with one set of documentation and sensible names that fit together and make a unified whole. Writing an asynchronous native node.js addon should not even require knowing that libuv exists or what it is.

boingoing · 2017-04-06T20:08:58Z

Would it be reasonable to use unique_ptr instead of handing out raw pointers in the Work class? Is there an ABI issue?

@kkoopa There isn't an ABI issue blocking us from using unique_ptr wrapping Work instances and handing those out as napi_async_work pointers, if that's what you meant. Since the Work class is not exposed outside of napi internals, we aren't worried about users messing-up the state via these opaque napi_async_work pointers. We use the same pattern for returning Reference instances via napi_ref opaque pointers via napi_create_reference. That's the main reason I have used the same pattern here.

boingoing · 2017-04-06T20:11:12Z

The issue and comments @mhdawson is referencing above are at nodejs/abi-stable-node#204 (comment)

kkoopa · 2017-04-06T20:24:59Z

Yes that is what I was wondering about. If the class is not exposed, it is fine as is.

…

On April 6, 2017 11:09:32 PM GMT+03:00, Taylor Woll ***@***.***> wrote: >Would it be reasonable to use unique_ptr instead of handing out raw pointers in the Work class? Is there an ABI issue? @kkoopa There isn't an ABI issue blocking us from using `unique_ptr` wrapping `Work` instances and handing those out as `napi_async_work` pointers, if that's what you meant. Since the `Work` class is not exposed outside of napi internals, we aren't worried about users messing-up the state via these opaque `napi_async_work` pointers. We use the same pattern for returning `Reference` instances via `napi_ref` opaque pointers via `napi_create_reference`. That's the main reason I have used the same pattern here. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #12250 (comment)

digitalinfinity · 2017-04-06T20:34:50Z

test/addons-napi/test_async/test_async.cc

Nit: redundant space before function

It is not redundant, it is part of the string on the line above.

digitalinfinity · 2017-04-06T20:36:38Z

test/addons-napi/test_async/test_async.cc

Can you have this test case queue the work multiple times and verify that that works? You'll probably have to change the complete handler to not delete the work after every item but only when all work has been completed

TimothyGu · 2017-04-07T05:54:32Z

src/node_api_types.h

Why was this moved?

I put the napi_callback typedef near napi_async_execute_callback and other callback typedefs. But napi_async_complete_callback takes a napi_status so all the callbacks should be located down below the enum (at least). However, napi_property_descriptor has napi_callback members so it also had to move down in the file.

TimothyGu · 2017-04-08T01:15:09Z

test/addons-napi/test_async/test_async.cc

Either
errorMessage = errorMessage == nullptr ? "empty error message" : errorMessage;
or
errorMessage = errorMessage || "empty error message";

Good point. This will be refactored via #12248 so I didn't pull all of that in for this change. Will make this change, though.

TimothyGu · 2017-04-08T01:17:11Z

test/addons-napi/test_async/test_async.cc

We use snake_case for C++ code.

Thanks for noticing. I fixed this via 1b210fe

mhdawson · 2017-04-10T14:26:51Z

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

mhdawson · 2017-04-10T14:30:41Z

Link failures: ln -s /usr/local/bin/node

[node-test-linter] $ /bin/sh -xe /tmp/hudson8032403002558779263.sh + gmake lint-ci ./node tools/jslint.js -j 2 -f tap -o test-eslint.tap \ benchmark lib test tools test/addons-napi/test_async/test_async.cc:36: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] test/addons-napi/test_async/test_async.cc:44: private: should be indented +1 space inside struct AutoHandleScope [whitespace/indent] [3] test/addons-napi/test_async/test_async.cc:58: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4] test/addons-napi/test_async/test_async.cc:63: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4] test/addons-napi/test_async/test_async.cc:75: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4] test/addons-napi/test_async/test_async.cc:82: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4] test/addons-napi/test_async/test_async.cc:92: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4] Total errors found: 7 gmake: *** [Makefile:884: cpplint] Error 1 Build step 'Execute shell' marked build as failure Run condition [Always] enabling perform for step [[Execute a set of scripts]] [PostBuildScript] - Execution post build scripts. [PostBuildScript] Build is not success : do not execute script Run condition [Always] enabling perform for step [[Execute a set of scripts]] [PostBuildScript] - Execution post build scripts. [node-test-linter] $ /bin/sh -xe /tmp/hudson1858009741163367606.sh + set +x Notifying upstream projects of job completion Finished: FAILURE

Please fix up and make sure to run make lint

boingoing · 2017-04-10T19:21:35Z

@mhdawson Pushed 85ca55a to fix these lint errors. Are the same rules used when we run vcbuild.cmd test? Because the lint run I did after 3556e92 was clean on my machine.

Anyway, could you kick off a new CI, please? :)

mhdawson · 2017-04-10T20:41:00Z

I assume it should be the same across platforms so not sure what might have happend:

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

mhdawson · 2017-04-10T21:44:39Z

Sorry your other PR made it in first can you do a rebase and the I'll give it one more shot.

Based on the async methods we had in abi-stable-node before the napi feature landed in node/master. Changed this set of APIs to handle error cases and removed a lot of the extra methods we had for setting all the pieces of napi_work opting instead to pass all of those as arguments to napi_create_async_work as none of those parameters are optional except for the complete callback, anyway. Renamed the napi_work struct to napi_async_work and replace the struct itself with a class which can better encapsulate the object lifetime and uv_work_t that we're trying to wrap anyway. Added a napi_async_callback type for the async helper callbacks instead of taking raw function pointers and make this callback take a napi_env parameter as well as the void* data it was already taking. Call the complete handler for the async work item with a napi_status code translated from the uvlib error code. The execute callback is required for napi_create_async_work, though complete callback is still optional. Also added some async unit tests for addons-napi based on the addons/async_hello_world test.

boingoing · 2017-04-10T22:04:32Z

@mhdawson No worry, I knew one of them would get in first which would conflict with the other. I did the rebase and running test locally to see if I can catch any kind of lint errors on my end before updating the PR.

mhdawson · 2017-04-10T22:23:18Z

ok, If you can do it today I'll check in later tonight to see if we can get it landed as I'm pretty booked tomorrow.

boingoing · 2017-04-10T22:44:26Z

@mhdawson Thanks for baby-sitting this PR. I pushed the rebase via 35a0287.

addaleax · 2017-04-10T22:45:47Z

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

addaleax · 2017-04-10T23:41:52Z

Landed in 9decfb1

Based on the async methods we had in abi-stable-node before the napi feature landed in node/master. Changed this set of APIs to handle error cases and removed a lot of the extra methods we had for setting all the pieces of napi_work opting instead to pass all of those as arguments to napi_create_async_work as none of those parameters are optional except for the complete callback, anyway. Renamed the napi_work struct to napi_async_work and replace the struct itself with a class which can better encapsulate the object lifetime and uv_work_t that we're trying to wrap anyway. Added a napi_async_callback type for the async helper callbacks instead of taking raw function pointers and make this callback take a napi_env parameter as well as the void* data it was already taking. Call the complete handler for the async work item with a napi_status code translated from the uvlib error code. The execute callback is required for napi_create_async_work, though complete callback is still optional. Also added some async unit tests for addons-napi based on the addons/async_hello_world test. PR-URL: #12250 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

mhdawson · 2017-04-11T00:56:16Z

:)

boingoing · 2017-04-11T01:10:27Z

Thanks all! 😃

aruneshchandra · 2017-04-11T22:51:35Z

do we need to tag this with the right labels to be picked up for next RC ?

Based on the async methods we had in abi-stable-node before the napi feature landed in node/master. Changed this set of APIs to handle error cases and removed a lot of the extra methods we had for setting all the pieces of napi_work opting instead to pass all of those as arguments to napi_create_async_work as none of those parameters are optional except for the complete callback, anyway. Renamed the napi_work struct to napi_async_work and replace the struct itself with a class which can better encapsulate the object lifetime and uv_work_t that we're trying to wrap anyway. Added a napi_async_callback type for the async helper callbacks instead of taking raw function pointers and make this callback take a napi_env parameter as well as the void* data it was already taking. Call the complete handler for the async work item with a napi_status code translated from the uvlib error code. The execute callback is required for napi_create_async_work, though complete callback is still optional. Also added some async unit tests for addons-napi based on the addons/async_hello_world test. PR-URL: nodejs#12250 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

Based on the async methods we had in abi-stable-node before the napi feature landed in node/master. Changed this set of APIs to handle error cases and removed a lot of the extra methods we had for setting all the pieces of napi_work opting instead to pass all of those as arguments to napi_create_async_work as none of those parameters are optional except for the complete callback, anyway. Renamed the napi_work struct to napi_async_work and replace the struct itself with a class which can better encapsulate the object lifetime and uv_work_t that we're trying to wrap anyway. Added a napi_async_callback type for the async helper callbacks instead of taking raw function pointers and make this callback take a napi_env parameter as well as the void* data it was already taking. Call the complete handler for the async work item with a napi_status code translated from the uvlib error code. The execute callback is required for napi_create_async_work, though complete callback is still optional. Also added some async unit tests for addons-napi based on the addons/async_hello_world test. Backport-PR-URL: #19447 PR-URL: #12250 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 6, 2017

mscdex added the node-api Issues and PRs related to the Node-API. label Apr 6, 2017

mhdawson approved these changes Apr 6, 2017
View reviewed changes

digitalinfinity reviewed Apr 6, 2017
View reviewed changes

digitalinfinity approved these changes Apr 6, 2017
View reviewed changes

addaleax self-requested a review April 6, 2017 21:22

TimothyGu reviewed Apr 7, 2017
View reviewed changes

TimothyGu reviewed Apr 8, 2017
View reviewed changes

addaleax approved these changes Apr 9, 2017
View reviewed changes

TimothyGu approved these changes Apr 9, 2017
View reviewed changes

boingoing mentioned this pull request Apr 10, 2017
napi: change napi_callback to return napi_value #12248
Closed
3 tasks

boingoing force-pushed the async_api branch from 85ca55a to 35a0287Compare April 10, 2017 22:43

boingoing changed the title ~~napi: implement async helper methods~~n-api: implement async helper methodsApr 10, 2017

addaleax closed this Apr 10, 2017

addaleax added dont-land-on-v4.x labels Apr 13, 2017

jasnell mentioned this pull request May 11, 2017
8.0.0 Release Proposal #12220
Closed

MylesBorins mentioned this pull request Apr 16, 2018
v6.14.2 proposal #19996
Merged

Uh oh!

n-api: implement async helper methods#12250

n-api: implement async helper methods #12250

Uh oh!

Conversation

boingoing commented Apr 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

boingoing commented Apr 6, 2017

Uh oh!

kkoopa commented Apr 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Apr 6, 2017

Uh oh!

addaleax commented Apr 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Apr 6, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boingoing commented Apr 6, 2017

Uh oh!

boingoing commented Apr 6, 2017

Uh oh!

kkoopa commented Apr 6, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Apr 10, 2017

Uh oh!

mhdawson commented Apr 10, 2017

Uh oh!

boingoing commented Apr 10, 2017

Uh oh!

mhdawson commented Apr 10, 2017

Uh oh!

mhdawson commented Apr 10, 2017

Uh oh!

boingoing commented Apr 10, 2017

Uh oh!

mhdawson commented Apr 10, 2017

Uh oh!

boingoing commented Apr 10, 2017

Uh oh!

addaleax commented Apr 10, 2017

Uh oh!

addaleax commented Apr 10, 2017

Uh oh!

mhdawson commented Apr 11, 2017

Uh oh!

boingoing commented Apr 11, 2017

Uh oh!

aruneshchandra commented Apr 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

boingoing commented Apr 6, 2017•
edited
Loading

kkoopa commented Apr 6, 2017•
edited
Loading

addaleax commented Apr 6, 2017•
edited
Loading

mhdawson commented Apr 6, 2017•
edited
Loading