GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — n-api: implement async helper methods by boingoing · Pull Request #12250 · nodejs/node

Conversation

@boingoing

@boingoing boingoing commented Apr 6, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@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 Node-API. label Apr 6, 2017
@boingoing

boingoing commented Apr 6, 2017

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kkoopa

kkoopa commented Apr 6, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Apr 6, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax

addaleax commented Apr 6, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhdawson
mhdawson approved these changes Apr 6, 2017
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mhdawson

mhdawson commented Apr 6, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@boingoing

boingoing commented Apr 6, 2017

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@boingoing

boingoing commented Apr 6, 2017

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kkoopa

kkoopa commented Apr 6, 2017 via email

Copy link
Copy Markdown
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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

digitalinfinity
digitalinfinity reviewed Apr 6, 2017
View reviewed changes
Comment thread test/addons-napi/test_async/test_async.cc Outdated

@digitalinfinity digitalinfinity Apr 6, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@kkoopa kkoopa Apr 6, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
digitalinfinity
digitalinfinity reviewed Apr 6, 2017
View reviewed changes
Comment thread test/addons-napi/test_async/test_async.cc Outdated

@digitalinfinity digitalinfinity Apr 6, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
digitalinfinity
digitalinfinity approved these changes Apr 6, 2017
View reviewed changes
@addaleax
addaleax self-requested a review April 6, 2017 21:22
TimothyGu
TimothyGu reviewed Apr 7, 2017
View reviewed changes
Comment thread src/node_api_types.h Outdated

@TimothyGu TimothyGu Apr 7, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why was this moved?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@boingoing boingoing Apr 7, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

TimothyGu reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
TimothyGu
TimothyGu reviewed Apr 8, 2017
View reviewed changes
Comment thread test/addons-napi/test_async/test_async.cc Outdated

@TimothyGu TimothyGu Apr 8, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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";

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@boingoing boingoing Apr 8, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/addons-napi/test_async/test_async.cc Outdated

@TimothyGu TimothyGu Apr 8, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@boingoing boingoing Apr 8, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
addaleax
addaleax approved these changes Apr 9, 2017
View reviewed changes
TimothyGu
TimothyGu approved these changes Apr 9, 2017
View reviewed changes
@mhdawson

mhdawson commented Apr 10, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhdawson

mhdawson commented Apr 10, 2017

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@boingoing

boingoing commented Apr 10, 2017

Copy link
Copy Markdown
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? :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

mhdawson commented Apr 10, 2017

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhdawson

mhdawson commented Apr 10, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@boingoing
n-api: implement async helper methods
35a0287
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

boingoing commented Apr 10, 2017

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhdawson

mhdawson commented Apr 10, 2017

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

boingoing commented Apr 10, 2017

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

addaleax commented Apr 10, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax

addaleax commented Apr 10, 2017

Copy link
Copy Markdown
Member

Landed in 9decfb1

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax addaleax closed this Apr 10, 2017
addaleax pushed a commit that referenced this pull request Apr 10, 2017
@boingoing @addaleax
n-api: implement async helper methods
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

mhdawson commented Apr 11, 2017

Copy link
Copy Markdown
Member

:)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@boingoing

boingoing commented Apr 11, 2017

Copy link
Copy Markdown
Contributor Author

Thanks all! 😃

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aruneshchandra

aruneshchandra commented Apr 11, 2017

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax addaleax added dont-land-on-v4.x labels Apr 13, 2017
@jasnell jasnell mentioned this pull request May 11, 2017
8.0.0 Release Proposal #12220
Closed
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
@boingoing Gabriel Schulhof
n-api: implement async helper methods
257b28c
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
@boingoing @MylesBorins
n-api: implement async helper methods
d82fd2a
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
v6.14.2 proposal #19996
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
+4 more reviewers
@kkoopa kkoopa kkoopa left review comments
@digitalinfinity digitalinfinity digitalinfinity approved these changes
@TimothyGu TimothyGu TimothyGu approved these changes
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@boingoing @kkoopa @bnoordhuis @addaleax @mhdawson @aruneshchandra @digitalinfinity @TimothyGu @mscdex @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.