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

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

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

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

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.

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 :)

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.

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.

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

This would need to be rebased again, sorry.

@addaleax NP. Rebasing when I get a chance.

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

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.

@addaleax Done.

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

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

Landed in 7efb8f7.