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: implement async helper methods #12250

Closed
wants to merge 1 commit into from

Conversation

boingoing
Copy link
Contributor

@boingoing boingoing commented Apr 6, 2017

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 6, 2017
@mscdex mscdex added the node-api Issues and PRs related to the Node-API. label Apr 6, 2017
@boingoing
Copy link
Contributor Author

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

@kkoopa
Copy link

kkoopa commented Apr 6, 2017

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

@bnoordhuis
Copy link
Member

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

@addaleax
Copy link
Member

addaleax commented Apr 6, 2017

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

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 Apr 6, 2017

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@kkoopa
Copy link

kkoopa commented Apr 6, 2017 via email

NAPI_CHECK(env, t == napi_number, "Wrong first argument, integer expected.");
NAPI_CALL(env, napi_typeof(env, argv[1], &t));
NAPI_CHECK(env, t == napi_function, "Wrong second argument,"
" function expected.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: redundant space before function

Copy link

Choose a reason for hiding this comment

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

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

NAPI_CALL(env, napi_create_async_work(
env, Execute, Complete, &theCarrier, &theCarrier._request));
NAPI_CALL(env,
napi_queue_async_work(env, theCarrier._request));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

napi_property_attributes attributes;
void* data;
} napi_property_descriptor;
Copy link
Member

Choose a reason for hiding this comment

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

Why was this moved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const napi_extended_error_info* error; \
napi_get_last_error_info((env), &error); \
const char *errorMessage = error->error_message; \
errorMessage = errorMessage ? errorMessage : "empty error message"; \
Copy link
Member

Choose a reason for hiding this comment

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

Either

errorMessage = errorMessage == nullptr ? "empty error message" : errorMessage;

or

errorMessage = errorMessage || "empty error message";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

napi_async_work _request;
} carrier;

carrier theCarrier;
Copy link
Member

Choose a reason for hiding this comment

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

We use snake_case for C++ code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing. I fixed this via 1b210fe

@mhdawson
Copy link
Member

@mhdawson
Copy link
Member

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
Copy link
Contributor Author

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

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

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
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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

@boingoing boingoing changed the title napi: implement async helper methods n-api: implement async helper methods Apr 10, 2017
@addaleax
Copy link
Member

@addaleax
Copy link
Member

Landed in 9decfb1

@addaleax addaleax closed this Apr 10, 2017
addaleax pushed a commit that referenced this pull request Apr 10, 2017
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
Copy link
Member

:)

@boingoing
Copy link
Contributor Author

Thanks all! 😃

@aruneshchandra
Copy link

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

@jasnell jasnell mentioned this pull request May 11, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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]>
@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++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants