n-api: implement promise #14365

gabrielschulhof · 2017-07-19T12:28:08Z

Promise is implemented as a pair of objects. napi_create_promise()
returns both a JavaScript promise and a "deferred" in its out-params.
The deferred is linked to the promise such that the deferred can be
passed to napi_conclude_deferred() to rejct/resolve the promise. The
deferred is a valid JavaScript value, but it is a shell object offering
no useful JavaScript functionality. In contrast, the promise returned
by napi_create_promise() is a full-fledged native JavaScript Promise
which can be used for chaining in JavaScript.

napi_is_promise() can be used to check if a napi_value is a native
promise - that is, a promise created by the underlying engine, rather
than a pure JS implementation of a promise.

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

TimothyGu

The deferred wrapping is beautiful. LGTM, except a few nits.

TimothyGu · 2017-07-19T13:26:23Z

src/node_api.cc

+                             v8::Local<v8::Object>* result) {
+  auto wrapper_template = EnvObjectTemplate(env->isolate, obj_template, 2);
+  auto maybe_object =
+    wrapper_template->NewInstance(env->isolate->GetCurrentContext());


Four-space indentation please.

TimothyGu · 2017-07-19T13:51:03Z

src/node_api.cc

+NAPI_EXTERN napi_status napi_conclude_deferred(napi_env env,
+                                               napi_value deferred,
+                                               napi_value conclusion,
+                                               bool is_resolution) {


I'd prefer is_fulfilled, as "resolution" can be interpreted as synonymous with "conclusion", generally speaking.

TimothyGu · 2017-07-19T13:51:23Z

src/node_api.cc

+
+  v8::Local<v8::Object> v8_deferred;
+  CHECK_TO_OBJECT(env, context, v8_deferred,
+    v8impl::JsValueFromV8LocalValue(js_deferred->GetInternalField(0)));


Indentation... and also a few lines below.

TimothyGu · 2017-07-19T13:53:14Z

test/addons-napi/test_promise/test_promise.c

+  napi_property_descriptor descriptors[] = {
+    DECLARE_NAPI_PROPERTY("createPromise", createPromise),
+    DECLARE_NAPI_PROPERTY("concludeCurrentPromise", concludeCurrentPromise),
+    DECLARE_NAPI_PROPERTY("isPromise", isPromise),


There should be an anti-case for passing a promise to napi_conclude_deferred().

TimothyGu · 2017-07-19T14:08:47Z

This PR inspired me to think of a potentially better idea for object wrapping. Please take a look: #14367

benjamingr · 2017-07-19T15:08:17Z

doc/api/n-api.md

+Promises facilitate the construction of chains of asynchronous actions such
+that each action hinges on the outcome of the previous action.
+
+N-API implements promises as a pair of two objects - a "deferred" and a


Maybe describe (quickly) their motivation? A "deferred" - which is in charge of resolving the promise

benjamingr · 2017-07-19T15:10:17Z

doc/api/n-api.md

@@ -3155,6 +3156,81 @@ support it:
 * If the function is not available, provide an alternate implementation
  that does not use the function.

+## Promises
+
+Promises facilitate the construction of chains of asynchronous actions such


This is broad and I'm not sure it's very helpful to users.

I prefer a more informational tone about what promises are in N-API and not what they enable users to do.

"A promise represents an eventual value - the result of an asynchronous action. A promise can be awaited on JavaScript's side or a callback could be attached to it".

The chaining aspect is interesting, but not really related to n-api from what I know.

benjamingr · 2017-07-19T15:11:58Z

doc/api/n-api.md

+```
+
+- `[in] env`: The environment that the API is invoked under.
+- `[out] deferred`: The deferred object.


"A deferred object in charge of resolving the promise" perhaps?

benjamingr · 2017-07-19T15:12:48Z

doc/api/n-api.md

+
+Returns `napi_ok` if the API succeeded.
+
+This API creates a deferred object and a JavaScript promise. The two objects


This paragraph repeats the one at the top a little (especially the start).

I guess I feel like I cannot sufficiently emphasize that it is not the promise one resolves, but the deferred.

I think that if we fix the motivation part above it will get a lot clearer.

benjamingr · 2017-07-19T15:12:59Z

doc/api/n-api.md

+the functionality of a JavaScript promise. However, the JavaScript promise
+returned in the third parameter can be passed into JavaScript.
+
+### napi_conclude_deferred


A usage example would be amazing)

benjamingr · 2017-07-19T15:15:09Z

doc/api/n-api.md

+- `[in] env`: The environment that the API is invoked under.
+- `[in] deferred`: The deferred object whose associated promise to conclude.
+- `[in] conclusion`: The value with which to conclude the promise.
+- `[in] is_rejection`: Flag indicating whether `conclusion` is a resolution


Wouldn't it be better to have napi_promise_fulfill(env, deferred, value) and napi_promise_reject(env, deferred, value)?

When I think about it - why do we even need a deferred object at all?

benjamingr · 2017-07-19T15:16:19Z

doc/api/n-api.md

+
+- `[in] env`: The environment that the API is invoked under.
+- `[in] promise`: The promise to examine
+- `[out] is_promise`: Flag indicating whether `promise` is a native promise


As someone who has helped maintain large promise libraries - I'm totally fine with making people Promise.resolve promises they pass to napi_* methods.

What do you mean by "I'm totally fine making people Promise.resolve promises"? Do you mean that you don't mind if the native side resolves a promise created in JS and then passed to the native side?

@gabrielschulhof I mean I'm fine with is_promise only caring about and working with native promises - and ignoring non-native promises in n-api completely

benjamingr

I started reviewing it - but I'm not sure about several more general things API wise in this PR - the API we're exposing seems complicated.

What about exporting the following instead?

napi_create_promise(env, *promise); // no deferred
napi_promise_resolve(env, *promise, *value); 
napi_promise_reject(env, *promise, *reason);

We also need to consider what happens if a promise is passed as a fulfillment value (the state should be assimilated).

TimothyGu · 2017-07-19T15:44:10Z

@benjamingr Some context about the deferred object is in an earlier iteration of the PR (#13717). +1 about nested promises though, going to need some tests for that.

gabrielschulhof · 2017-07-19T15:51:32Z

@benjamingr the V8 implementation provides two objects: a resolver and a promise. Now, it's true that they are in reality one and the same object, but that's an implementation detail that the API does not document. The v8 API certainly suggests that, given a random v8::Promise we need the corresponding v8::Promise::Resolver to resolve it.

So, if somebody passes in a v8::Promise from the JS side, we must be able to somehow obtain its v8::Promise::Resolver in order to resolve it. We must not make use of the implementation fact that the v8::Promise is the v8::Promise::Resolver because V8 might later change on us.

Now, if we hide the v8::Promise::Resolver inside the v8::Promise using our napi_wrap() technique when we create the v8::Promise in napi_create_promise() then we give the appearance that one can resolve a v8::Promise - any v8::Promise - via napi_resolve_promise(). That's not true though, because internally we'd be retrieving the v8::Promise::Resoslver in order to perform the resolve/reject. Now, v8::Promise instances not created using napi_create_promise() would not have a v8::Promise::Resolver hidden inside them, and so we'd have to error out when the application attempted to resolve a promise not created with napi_create_promise(). This would be confusing to users.

Thus, If we are not to covey the impression that napi_create_promise() creates a promise which can then be passed to napi_resolve_promise() or napi_reject_promise() then we need to introduce the concept of a deferred in order to distinguish the promises we can resolve from those we cannot.

gabrielschulhof · 2017-07-19T15:56:42Z

@benjamingr another unpleasant side-effect of allowing one to resolve any promise natively is this:

addon.acceptPromise( new Promise( function( resolve, reject ) {
  ...
  resolve();
  ...
} ) );

and then

napi_value the_passed_in_promise;
...
napi_promise_resolve(env, the_passed_in_promise, the_result);

Now, the JS function responsible for resolving the promise is completely shoved aside. In fact, we don't know whether the promise will be resolved by the native side or by the JS side - depends on which finishes first.

benjamingr · 2017-07-20T07:33:36Z

So, if somebody passes in a v8::Promise from the JS side, we must be able to somehow obtain its v8::Promise::Resolver in order to resolve it.

Yes, we can keep a map of native promises and resolvers for unresolved promises - when a promise is resolved we remove it from the map.

Now, if we hide the v8::Promise::Resolver inside the v8::Promise using our napi_wrap() technique when we create the v8::Promise in napi_create_promise() then we give the appearance that one can resolve a v8::Promise - any v8::Promise - via napi_resolve_promise()

That's a good point, although we can return a ResolvablePromise or some type that gives us this capability.

In fact, we don't know whether the promise will be resolved by the native side or by the JS side - depends on which finishes first.

This is how promises work though. The first resolver wins - I'd assume someone writing that sort of code is using this as a feature and not running into it as a bug.

benjamingr · 2017-07-20T08:03:08Z

Thanks a lot for working on this by the way - I'm sorry I'm not familiar with all the previous discussion and intend to read it asap. I care about having a convenient n_api promise interface because I intend to use it a lot :)

gabrielschulhof · 2017-07-20T09:14:25Z

In fact, we don't know whether the promise will be resolved by the native side or by the JS side - depends on which finishes first.

This is how promises work though. The first resolver wins - I'd assume someone writing that sort of code is using this as a feature and not running into it as a bug.

Wait, what? Multiple resolvers? I mean, OK, Promise.race() behaves like that, but a single promise? My understanding is that given a promise, it can be resolved in exactly one place, namely inside its executor. In JS this is solved by making the resolve and reject callbacks arguments to the executor. I mean, you can leak the resolve and reject callbacks by having the executor assign them as properties on the promise object, but that's pretty ... I dunno ... hackish?

Now, on the native side we do not have the luxury of using scope to at least discourage people from passing around the resolve and reject callbacks (collected in the deferred in the N-API implementation), but we still have the same concept of a single resolver being associated with a promise.

That's why I believe it would be confusing if a user created a Promise in JS and found that its conclusion was not the result of having performed whatever was given in the executor.

Yes, we can keep a map of native promises and resolvers for unresolved promises - when a promise is resolved we remove it from the map.

We are already storing a lot of things in the napi_env, and if we add this map we expose promises to the issue currently facing napi_env which is that each module has its own napi_env, so one module couldn't resolve another module's promises because they wouldn't be in its map.

Also, users who pass a JS-created native promise into napi_resolve_promise() would continue to find it confusing that napi_resolve_promise() fails because internally the promise they passed does not map to any resolver because it wasn't created with napi_create_promise().

I guess the fundamental questions are: Do we value the abstraction imposed by JS wherein upon receiving a Promise object from somewhere, the receiver cannot resolve it, but can only wait for its conclusion? Do we value that the promise can only be resolved by its executor, unless the user jumps through some (to-be-frowned-upon?) hoops of assigning the resolve and reject to the Promise instance?

If so, then we should seek to encourage code that keeps native code from resolving JS promises, and keeps JS code from resolving native promises.

benjamingr · 2017-07-20T09:24:09Z

Wait, what? Multiple resolvers? I mean, OK, Promise.race() behaves like that, but a single promise? My understanding is that given a promise, it can be resolved in exactly one place, namely inside its executor. In JS this is solved by making the resolve and reject callbacks arguments to the executor

Making resolve/reject executor arguments was done for throw safety (so synchronous throws convert to rejections in the promise constructor).

new Promise((resolve, reject) => { // valid JS
  resolve(3);
  resolve(5); // noop
  reject(null); // also noop, only the first one counts
});

This is by design, and in fact how you'd implement race. I don't mind forbidding it in the n-api side but it would certainly not be the default and expected behavior for promise resolution.

I mean, you can leak the resolve and reject callbacks by having the executor assign them as properties on the promise object, but that's pretty ... I dunno ... hackish?

Let me clarify - the JS side should not be able to resolve the promise, what I'm suggesting is returning the promise object to the JS side (it can't resolve it) and resolving it only on the V8 side (since V8 can resolve on promise objects - or we can keep a mapping).

That's why I believe it would be confusing if a user created a Promise in JS and found that its conclusion was not the result of having performed whatever was given in the executor.

Simple - don't allow resolving promises that were created outside of n-api (or ones you don't have in the map).

so one module couldn't resolve another module's promises because they wouldn't be in its map.

That sounds like a pretty good idea to me regardless .- similarly to JS not resolving these promises. (I think?)

I guess the fundamental questions are: Do we value the abstraction imposed by JS wherein upon receiving a Promise object from somewhere, the receiver cannot resolve it, but can only wait for its conclusion?

I think we should.

Do we value that the promise can only be resolved by its executor, unless the user jumps through some (to-be-frowned-upon?) hoops of assigning the resolve and reject to the Promise instance?

These hooks were never frowned upon, the promise constructor was designed to run synchronously precisely so these use cases would be enabled. That said - I'm 100% for having JS users only resolve promises through the executor (and hopefully through util.promisify which resolves without an executor - but that's another story).

Basically, my main issue with this PR is that we have an extra type for a capability (which is something that is often done in OO) - but we're writing C so we don't really need that - since simply access to napi_resolve_promise would mean we're "priviledged" (and have the capability).

Your point about this meaning we're able to resolve promises we did not create is very good which is why I think the map is a good idea. That said, having a deferred is nice because it converts a would-be runtime error to a compile time error.

I'd like to think about having a nicer API for a day or two (we can also solve it with a resolvable-promise type - but I'm not sure that's better.

gabrielschulhof · 2017-07-21T10:13:30Z

@benjamingr

That said, having a deferred is nice because it converts a would-be runtime error to a compile time error.

Actually, it currently doesn't convert it to a compile time error because a deferred is currently simply a napi_value - although we could introduce a type for it which internally would store a persistent to the deferred, and would free it upon resolve/reject.

addaleax · 2017-07-30T11:23:43Z

This would need to be rebased again, sorry.

gabrielschulhof · 2017-08-03T20:39:30Z

@addaleax NP. Rebasing when I get a chance.

gabrielschulhof · 2017-08-04T21:29:05Z

@benjamingr have you had a chance to think about a nicer API? Is it OK to proceed with this API?

benjamingr · 2017-08-05T13:10:03Z

Oh sorry, I thought you were going to follow up with:

Actually, it currently doesn't convert it to a compile time error because a deferred is currently simply a napi_value - although we could introduce a type for it which internally would store a persistent to the deferred, and would free it upon resolve/reject.

I didn't realize you were waiting for me - if you'd like I can sit on the API this week.

gabrielschulhof · 2017-08-24T17:21:56Z

@addaleax Done.

TimothyGu · 2017-08-25T00:08:18Z

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

gabrielschulhof · 2017-08-25T09:00:36Z

Addressing the failure of node-test-binary-arm: https://ci.nodejs.org/job/node-test-binary-arm/9844/

gabrielschulhof · 2017-08-25T09:06:06Z

Landed in 7efb8f7.

Promise is implemented as a pair of objects. `napi_create_promise()` returns both a JavaScript promise and a newly allocated "deferred" in its out-params. The deferred is linked to the promise such that the deferred can be passed to `napi_resolve_deferred()` or `napi_reject_deferred()` to reject/resolve the promise. `napi_is_promise()` can be used to check if a `napi_value` is a native promise - that is, a promise created by the underlying engine, rather than a pure JS implementation of a promise. PR-URL: #14365 Fixes: nodejs/abi-stable-node#242 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Promise is implemented as a pair of objects. `napi_create_promise()` returns both a JavaScript promise and a newly allocated "deferred" in its out-params. The deferred is linked to the promise such that the deferred can be passed to `napi_resolve_deferred()` or `napi_reject_deferred()` to reject/resolve the promise. `napi_is_promise()` can be used to check if a `napi_value` is a native promise - that is, a promise created by the underlying engine, rather than a pure JS implementation of a promise. PR-URL: nodejs/node#14365 Fixes: nodejs/abi-stable-node#242 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Promise is implemented as a pair of objects. `napi_create_promise()` returns both a JavaScript promise and a newly allocated "deferred" in its out-params. The deferred is linked to the promise such that the deferred can be passed to `napi_resolve_deferred()` or `napi_reject_deferred()` to reject/resolve the promise. `napi_is_promise()` can be used to check if a `napi_value` is a native promise - that is, a promise created by the underlying engine, rather than a pure JS implementation of a promise. PR-URL: #14365 Fixes: nodejs/abi-stable-node#242 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Notable Changes * build: * Snapshots are now re-enabled in V8 #14875 * console: * Implement minimal `console.group()`. #14910 * deps: * upgrade libuv to 1.14.1 #14866 * update nghttp2 to v1.25.0 #14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. #14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. #15034 * inspector: * Enable async stack traces #13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` #14369 * napi: * implement promise #14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. #14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. #14680 * tls: * multiple PFX in createSecureContext [#14793](#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: #15308

Notable Changes * build: * Snapshots are now re-enabled in V8 nodejs#14875 * console: * Implement minimal `console.group()`. nodejs#14910 * deps: * upgrade libuv to 1.14.1 nodejs#14866 * update nghttp2 to v1.25.0 nodejs#14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. nodejs#14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. nodejs#15034 * inspector: * Enable async stack traces nodejs#13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` nodejs#14369 * napi: * implement promise nodejs#14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. nodejs#14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. nodejs#14680 * tls: * multiple PFX in createSecureContext [nodejs#14793](nodejs#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: nodejs#15308

Promise is implemented as a pair of objects. `napi_create_promise()` returns both a JavaScript promise and a newly allocated "deferred" in its out-params. The deferred is linked to the promise such that the deferred can be passed to `napi_resolve_deferred()` or `napi_reject_deferred()` to reject/resolve the promise. `napi_is_promise()` can be used to check if a `napi_value` is a native promise - that is, a promise created by the underlying engine, rather than a pure JS implementation of a promise. PR-URL: nodejs#14365 Fixes: nodejs/abi-stable-node#242 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Promise is implemented as a pair of objects. `napi_create_promise()` returns both a JavaScript promise and a newly allocated "deferred" in its out-params. The deferred is linked to the promise such that the deferred can be passed to `napi_resolve_deferred()` or `napi_reject_deferred()` to reject/resolve the promise. `napi_is_promise()` can be used to check if a `napi_value` is a native promise - that is, a promise created by the underlying engine, rather than a pure JS implementation of a promise. Backport-PR-URL: #19447 PR-URL: #14365 Fixes: nodejs/abi-stable-node#242 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Jul 19, 2017

gabrielschulhof force-pushed the napi-promise branch from 7de6cc2 to 26bc950 Compare July 19, 2017 12:33

TimothyGu reviewed Jul 19, 2017

View reviewed changes

benjamingr reviewed Jul 19, 2017

View reviewed changes

benjamingr previously requested changes Jul 19, 2017

View reviewed changes

gabrielschulhof force-pushed the napi-promise branch 2 times, most recently from 7de46a6 to 7eef77d Compare July 20, 2017 09:46

gabrielschulhof force-pushed the napi-promise branch from 7eef77d to 2ff291f Compare July 23, 2017 00:06

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 30, 2017

gabrielschulhof force-pushed the napi-promise branch from 2ff291f to 48f7deb Compare August 4, 2017 21:27

gabrielschulhof force-pushed the napi-promise branch from 8b35e05 to b16ac9e Compare August 24, 2017 17:21

TimothyGu approved these changes Aug 24, 2017

View reviewed changes

gabrielschulhof closed this Aug 25, 2017

gabrielschulhof deleted the napi-promise branch August 25, 2017 09:07

jasongin mentioned this pull request Sep 3, 2017

Add API for working with Promises nodejs/node-addon-api#126

Closed

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Jan 11, 2018

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged


		Returns `napi_ok` if the API succeeded.

		This API creates a deferred object and a JavaScript promise. The two objects

n-api: implement promise #14365

n-api: implement promise #14365

Conversation

gabrielschulhof commented Jul 19, 2017

Checklist

Affected core subsystem(s)

TimothyGu 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

TimothyGu commented Jul 19, 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

benjamingr left a comment

Choose a reason for hiding this comment

TimothyGu commented Jul 19, 2017 • edited Loading

gabrielschulhof commented Jul 19, 2017

gabrielschulhof commented Jul 19, 2017

benjamingr commented Jul 20, 2017

benjamingr commented Jul 20, 2017

gabrielschulhof commented Jul 20, 2017

benjamingr commented Jul 20, 2017

gabrielschulhof commented Jul 21, 2017 • edited Loading

addaleax commented Jul 30, 2017

gabrielschulhof commented Aug 3, 2017

gabrielschulhof commented Aug 4, 2017

benjamingr commented Aug 5, 2017

gabrielschulhof commented Aug 24, 2017

TimothyGu commented Aug 25, 2017

gabrielschulhof commented Aug 25, 2017

gabrielschulhof commented Aug 25, 2017

TimothyGu commented Jul 19, 2017 •

edited

Loading

gabrielschulhof commented Jul 21, 2017 •

edited

Loading