napi: change napi_callback to return napi_value #12248

boingoing · 2017-04-06T01:21:48Z

Change napi_callback to return napi_value directly instead of
requiring module code to call napi_set_return_value.

When we invoke the callback, we will check the return value and call
SetReturnValue ourselves. If the callback returns NULL, we don't
set the return value in v8 which would have the same effect as
previously if the callback didn't call napi_set_return_value. Seems
to be a more natural way to handle return values from callbacks. As a
consequence, remove napi_set_return_value.

Add a napi_value to napi_property_descriptor to support
string values which couldn't be passed in the utf8name parameter
or symbols as property names. Class names, however, cannot be symbols
so this napi_value must be a string type in that case.

Remove all of the napi_callback_info helpers except for
napi_get_cb_info and make all the parameters to napi_get_cb_info
optional. If argv is provided, argc will become non-optional.

Update all the test collateral according to these changes. Also add
test/addons-napi/common.h to house some common macros for wrapping
N-API calls and error handling.

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)

napi

boingoing · 2017-04-06T01:41:21Z

@jasongin @digitalinfinity Please take a look at this change if you have the time.

addaleax · 2017-04-06T12:44:11Z

test/addons-napi/2_function_arguments/binding.c

@@ -1,58 +1,37 @@
 #include <node_api.h>
+#include "..\common.h"


addaleax · 2017-04-06T12:44:40Z

test/addons-napi/3_callbacks/binding.c

@@ -1,51 +1,43 @@
 #include <node_api.h>
+#include "..\common.h"


ditto, same everywhere

Ah yup. Sorry, dev'd this on Windows. :) Thanks for catching.

Addressed in a new commit - 03c0644

mhdawson · 2017-04-06T13:28:46Z

test/addons-napi/common.h

+#define NAPI_RETVAL_NOTHING  // Intentionally blank #define
+
+#define GET_AND_THROW_LAST_ERROR(env)                                    \
+  const napi_extended_error_info* error;                                 \


Should this check if the error is reporting an Exception. IF so we probably want to rethrow that instead of throwing a new Error.

That is true though none of the tests we have are checking for this. If an exception is pending, we could clear and rethrow it or do nothing and leave it pending. Is that equivalent?

updating to leave as pending is the equivalent and makes the most sense to me.

mhdawson · 2017-04-06T13:32:14Z

src/node_api.h

-                                         napi_callback_info cbinfo,
-                                         napi_value* result);
+    napi_value* this_arg,  // [out] Receives the JS 'this' arg for the call
+    void** data);          // [out] Receives the data pointer for the callback.


I'll have to check so this is mostly a note to myself. We don't need napi_get_cb_args_length here because there is another version ?

We added the napi_get_cb_info API so that modules could avoid making several calls to get the callback details and do everything via a single method. The next question, of course, is why would we offer the other several APIs if there's already one which is a super set? Since this PR already changed all the test files to remove napi_set_return_value, it seemed to make sense to roll-in the change to remove all the napi_get_cb_* calls as well.

Ok makes sense in that context. The only reason might be if its faster to get one versus several of the values and its a common case where you only need one of them.

I believe the overhead of making the call itself greatly outweighs the actual cost of accessing these members of the callback info object.

digitalinfinity · 2017-04-06T20:48:42Z

src/node_api.cc

-    CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
+    v8::Local<v8::Name> property_name;
+
+    if (p->utf8name != nullptr) {


Consider refactoring this into a helper method to get the property name from property descriptor

Spent a bit doing this now. I want this helper to live in the v8impl namespace which is located above the helper macros like CHECK_NEW_FROM_UTF8 in the source. Looks like we'll need to move the helper macros above v8impl. Any reservations?

Sounds fine to me. That's probably a better place for the macros anyway. Though you'll probably also need to move up some helper functions like napi_set_last_error() (or just declare them).

Added this helper via e2519d7

digitalinfinity

LGTM

jasongin · 2017-04-06T21:01:42Z

src/node_api.cc


      if (!define_maybe.FromMaybe(false)) {
        return napi_set_last_error(env, napi_generic_failure);
      }
-    } else if (p->getter || p->setter) {
+    } else if (p->getter != nullptr || p->setter != nullptr) {


I made this same change at #12240, though I also moved around some lines at the same time. Whoever goes in second will have to merge.

Yeah, I noticed this change there, too. Fingers crossed it'll merge automatically.

#12240 landed, so now this PR needs to be rebased.

mhdawson · 2017-04-07T13:47:20Z

This will need a rebase as well.

Change ```napi_callback``` to return ```napi_value``` directly instead of requiring ```napi_set_return_value```. When we invoke the callback, we will check the return value and call ```SetReturnValue``` ourselves. If the callback returns ```NULL```, we don't set the return value in v8 which would have the same effect as previously if the callback didn't call ```napi_set_return_value```. Seems to be a more natural way to handle return values from callbacks. As a consequence, remove ```napi_set_return_value```. Add a ```napi_value``` to ```napi_property_descriptor``` to support string values which couldn't be passed in the ```utf8name``` parameter or symbols as property names. Class names, however, cannot be symbols so this ```napi_value``` must be a string type in that case. Remove all of the ```napi_callback_info``` helpers except for ```napi_get_cb_info``` and make all the parameters to ```napi_get_cb_info``` optional except for argc. Update all the test collateral according to these changes. Also add ```test/addons-napi/common.h``` to house some common macros for wrapping N-API calls and error handling.

boingoing · 2017-04-07T21:59:26Z

Rebased on top of node/master

addaleax

src/ changes LGTM, somewhat rubber-stamp-y LGTM for the test parts ;)

addaleax · 2017-04-09T12:10:13Z

src/node_api.cc

+    auto str_maybe = v8::String::NewFromUtf8(                            \
+        (env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \
+    CHECK_MAYBE_EMPTY((env), str_maybe, napi_generic_failure);           \
+    result = str_maybe.ToLocalChecked();                                 \


Can you also wrap result in parentheses here?

Sure. We probably didn't do that originally because result is the LHS but looks like that works fine so I'll make the change.

addaleax · 2017-04-09T12:13:17Z

src/node_api.cc

@@ -123,6 +187,22 @@ v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
  return local;
 }

+napi_status V8NameFromPropertyDescriptor(napi_env env,


How about either adding static inline here too (and for helpers in general), or – and I think I would prefer that – turning v8impl into an anonymous namespace?

I understand the benefit of static inline but why do you prefer an anonymous namespace? I think there's some value in clumping together the v8 helpers and naming that clump v8impl (though name could be more descriptive 👍). We might have other namespaces for other clumps of helpers like uvimpl, etc so having a relevant name helps to reason about these groups, no?

@boingoing Having an anon namespace would be nice because none of the symbols for the utilities here need to be exported, like static would indicate too. You could keep the v8impl/uvimpl namespaces and just wrap their entire content in a anon namespace if you like that for readability.

Also, just noticed, if you go with static inline can you re-align the arguments here? ;)

Oh I see what you meant about the namespaces. I inferred your comment earlier to mean that you just preferred an unnamed namespace. I'd prefer to punt that particular change into a later PR since I want to get this and #12250 wrapped up early this week.

Is the argument alignment here good? With static inline at the beginning of the line, it's overall too long to have the arguments on subsequent lines align with the left-paren so I just pushed the next lines over to the right. Could also take some of static inline napi_status onto it's own line or align all the arguments by moving the first onto it's own line. Doesn't seem to be a hard rule about how this works and we're pretty inconsistent about it, in general, so I just picked what looked nice to me. If you feel strongly about this, though, I don't mind changing it. 👍

If you feel strongly about this, though

I don’t, keep anything for later PRs that you want. :)

addaleax · 2017-04-09T12:14:30Z

src/node_api.cc

    v8::PropertyAttribute attributes =
-        v8impl::V8PropertyAttributesFromDescriptor(p);
+      v8impl::V8PropertyAttributesFromDescriptor(p);


Is this intentional? We usually do 4-space indents for statement continuations

Oh thanks for letting me know about that. I just thought this was an oversight.

addaleax · 2017-04-09T12:14:40Z

src/node_api.cc

@@ -809,7 +838,7 @@ napi_status napi_define_class(napi_env env,
        attributes);
    } else if (p->method != nullptr) {
      v8::Local<v8::Object> cbdata =
-          v8impl::CreateFunctionCallbackData(env, p->method, p->data);
+        v8impl::CreateFunctionCallbackData(env, p->method, p->data);


addaleax · 2017-04-09T12:26:12Z

test/addons-napi/6_object_wrap/myobject.cc

+  size_t argc = 1;
+  napi_value args[1];
+  napi_value _this;
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, &_this, NULL));


Can you prefer nullptr over NULL in C++ sources?

Yes, makes sense.

boingoing · 2017-04-09T21:45:54Z

@addaleax I pushed 7d899eb which I believe addresses your feedback. Thanks for taking a look.

mhdawson

LGTM

mhdawson · 2017-04-10T14:39:52Z

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

mhdawson · 2017-04-10T15:36:14Z

lint errors

[node-test-linter] $ /bin/sh -xe /tmp/hudson8160929318737150329.sh
+ gmake lint-ci
./node tools/jslint.js -j 2 -f tap -o test-eslint.tap \
	benchmark lib test tools
test/addons-napi/6_object_wrap/myobject.cc:82:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/6_object_wrap/myobject.cc:110:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/7_factory_wrap/myobject.cc:82:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/7_factory_wrap/binding.cc:18:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/8_passing_wrapped/binding.cc:39:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
test/addons-napi/8_passing_wrapped/binding.cc:40:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
Total errors found: 6
gmake: *** [Makefile:884: cpplint] Error 1

please fix up and make sure to run make test

boingoing · 2017-04-10T19:15:23Z

@mhdawson Did run vcbuild.cmd test but must have missed these latest whitespace issues. Can you please kick back off the CI for b47c9aa?

addaleax · 2017-04-10T20:28:59Z

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

I assume this is ready to land once CI comes back okay?

mhdawson · 2017-04-10T20:42:31Z

@addaleax yes should be ready to go. Would have landed earlier if the first CI run had passed.

addaleax · 2017-04-10T21:32:37Z

Landed in ca786c3

(@nodejs/build … any idea what’s up with the osx CI not running?)

Also, for future reference: It would be cool if you could use n-api as the subsystem label in the commit message and wrap lines at 72 characters. And for inline code a single backtick is sufficient, the full ``` is only needed for multi-line blocks. ;)

Change `napi_callback` to return `napi_value` directly instead of requiring `napi_set_return_value`. When we invoke the callback, we will check the return value and call `SetReturnValue` ourselves. If the callback returns `NULL`, we don't set the return value in v8 which would have the same effect as previously if the callback didn't call `napi_set_return_value`. Seems to be a more natural way to handle return values from callbacks. As a consequence, remove `napi_set_return_value`. Add a `napi_value` to `napi_property_descriptor` to support string values which couldn't be passed in the `utf8name` parameter or symbols as property names. Class names, however, cannot be symbols so this `napi_value` must be a string type in that case. Remove all of the `napi_callback_info` helpers except for `napi_get_cb_info` and make all the parameters to `napi_get_cb_info` optional except for argc. Update all the test collateral according to these changes. Also add `test/addons-napi/common.h` to house some common macros for wrapping N-API calls and error handling. PR-URL: #12248 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

boingoing · 2017-04-10T21:47:55Z

@addaleax Thanks very much for your help with this PR!

I used napi to mark the subsystem because I saw another PR use the same before and didn't know if we had settled on a proper name. Will adjust future PRs, thanks. 🆗

I tried really hard to make the commit message match the requirements so sorry if I got that wrong - does the 72 character limit include some spaces at the beginning which I didn't add? Do the commit message rules apply to all of the commits or just the final squashed commit (which is what I imagine you actually pushed) and did you pull the message from the PR or the commits themselves?

Also, yeah, I actually didn't realize one backtick worked for inline code until recently since I tend to use multi-line code blocks with syntax highlighting.

addaleax · 2017-04-10T21:52:36Z

does the 72 character limit include some spaces at the beginning which I didn't add?

No, but maybe something got messed up while editing the commit message? Most of the lines in 8f34553 are longer than 72 chars (git show 8f34553 should show that)

Do the commit message rules apply to all of the commits or just the final squashed commit (which is what I imagine you actually pushed) and did you pull the message from the PR or the commits themselves?

Heh, looks like I should have pulled it from the PR description. ;) But yes, just the final squashed commit(s) that get merged. What you do inside the PR is completely up to you – rebase, squash, amend, or add commits in any way that you think makes sense. :)

The earlier version `napi_callback` returns `void` but now is `napi_value`. The document of this section hasn't been modified. Fixes: nodejs#12248

The earlier version `napi_callback` returns `void` but now is `napi_value`. The document of this section hasn't been modified. PR-URL: #13570 Fixes: #12248 Fixes: #13562 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Change `napi_callback` to return `napi_value` directly instead of requiring `napi_set_return_value`. When we invoke the callback, we will check the return value and call `SetReturnValue` ourselves. If the callback returns `NULL`, we don't set the return value in v8 which would have the same effect as previously if the callback didn't call `napi_set_return_value`. Seems to be a more natural way to handle return values from callbacks. As a consequence, remove `napi_set_return_value`. Add a `napi_value` to `napi_property_descriptor` to support string values which couldn't be passed in the `utf8name` parameter or symbols as property names. Class names, however, cannot be symbols so this `napi_value` must be a string type in that case. Remove all of the `napi_callback_info` helpers except for `napi_get_cb_info` and make all the parameters to `napi_get_cb_info` optional except for argc. Update all the test collateral according to these changes. Also add `test/addons-napi/common.h` to house some common macros for wrapping N-API calls and error handling. PR-URL: nodejs#12248 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

The earlier version `napi_callback` returns `void` but now is `napi_value`. The document of this section hasn't been modified. PR-URL: nodejs#13570 Fixes: nodejs#12248 Fixes: nodejs#13562 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Change `napi_callback` to return `napi_value` directly instead of requiring `napi_set_return_value`. When we invoke the callback, we will check the return value and call `SetReturnValue` ourselves. If the callback returns `NULL`, we don't set the return value in v8 which would have the same effect as previously if the callback didn't call `napi_set_return_value`. Seems to be a more natural way to handle return values from callbacks. As a consequence, remove `napi_set_return_value`. Add a `napi_value` to `napi_property_descriptor` to support string values which couldn't be passed in the `utf8name` parameter or symbols as property names. Class names, however, cannot be symbols so this `napi_value` must be a string type in that case. Remove all of the `napi_callback_info` helpers except for `napi_get_cb_info` and make all the parameters to `napi_get_cb_info` optional except for argc. Update all the test collateral according to these changes. Also add `test/addons-napi/common.h` to house some common macros for wrapping N-API calls and error handling. Backport-PR-URL: #19447 PR-URL: #12248 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

The earlier version `napi_callback` returns `void` but now is `napi_value`. The document of this section hasn't been modified. Backport-PR-URL: #19447 PR-URL: #13570 Fixes: #12248 Fixes: #13562 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

addaleax reviewed Apr 6, 2017

View reviewed changes

mhdawson reviewed Apr 6, 2017

View reviewed changes

digitalinfinity reviewed Apr 6, 2017

View reviewed changes

digitalinfinity approved these changes Apr 6, 2017

View reviewed changes

jasongin reviewed Apr 6, 2017

View reviewed changes

boingoing force-pushed the napi_callback_return_napi_value branch from e2519d7 to 8f34553 Compare April 7, 2017 21:58

boingoing mentioned this pull request Apr 8, 2017

n-api: implement async helper methods #12250

Closed

3 tasks

addaleax approved these changes Apr 9, 2017

View reviewed changes

addaleax added this to the 8.0.0 milestone Apr 9, 2017

Respond to review feedback

7d899eb

mhdawson approved these changes Apr 10, 2017

View reviewed changes

Fix lint errors

b47c9aa

addaleax closed this Apr 10, 2017

italoacasas added the backport-to-v7.x label Apr 10, 2017

italoacasas added dont-land-on-v4.x and removed backport-to-v7.x labels Apr 10, 2017

addaleax added the dont-land-on-v6.x label Apr 13, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

XadillaX added a commit to XadillaX/node that referenced this pull request Jun 9, 2017

doc: fix out of date napi_callback doc

286979e

The earlier version `napi_callback` returns `void` but now is `napi_value`. The document of this section hasn't been modified. Fixes: nodejs#12248

XadillaX mentioned this pull request Jun 9, 2017

doc: fix out of date napi_callback doc #13570

Closed

3 tasks

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

		@@ -1,58 +1,37 @@
		#include <node_api.h>
		#include "..\common.h"

		@@ -1,51 +1,43 @@
		#include <node_api.h>
		#include "..\common.h"

napi: change napi_callback to return napi_value #12248

napi: change napi_callback to return napi_value #12248

Conversation

boingoing commented Apr 6, 2017 • edited

Checklist

Affected core subsystem(s)

boingoing commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalinfinity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Apr 7, 2017

boingoing commented Apr 7, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing commented Apr 9, 2017

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Apr 10, 2017

mhdawson commented Apr 10, 2017

boingoing commented Apr 10, 2017

addaleax commented Apr 10, 2017

mhdawson commented Apr 10, 2017

addaleax commented Apr 10, 2017

boingoing commented Apr 10, 2017

addaleax commented Apr 10, 2017

boingoing commented Apr 6, 2017 •

edited