941920 - Implement full Promise API in Promise.jsm

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Brandon Benvie [:benvie]]

If we're going to use Promise.jsm as a stopgap while waiting on additional development for DOM Promises, we should implement the full API. That means adding:

* Promise.cast, which coerces its argument to a promise, or returns the argument if it is already a promise.

* Promise.race, which returns a new promise which is settled in the same way as the first passed promise to settle.

* Promise.prototype.catch, which is the same as calling promise.then(undefined, onReject)


See https://github.com/domenic/promises-unwrapping/blob/master/README.md#promisecast--x-

Comment 1

[Brandon Benvie [:benvie]]

Attachment: promise-api.patch

Here's a tentative implementation of the rest of the API (depends on the patch from bug 941757). I'll add tests and clean this up if you want to add this API and think this is the right approach.

Comment 2

[Brandon Benvie [:benvie]]

Attachment: promise-api.patch

Make Promise.prototype.catch return the new promise.

Comment 3

[:Paolo Amadini]

Comment on attachment 8336466 [details] [diff] [review]
promise-api.patch

I have a few doubts about hurrying up too much here, maybe we should require Chrome consumers to live without a few features for a while. Again, feel free to check with Dave as I'll be away next week and I may or may not have Internet access.

As long as we have two Promise implementations in the tree, I'm wary about implementing "catch" for Chrome code. In case an Add-on SDK promise is handed off to code that uses "catch", things could break.

With regard to "Promise.cast", what is its advantage over "Promise.resolve"? In fact, differently from what the name suggests, it seems that this "cast" implementation does not even guarantee that the returned value is of our desired Promise type, if a "thenable" is passed as its argument.

I realize the latter could be a specification question, but I'm not sure about how much the spec is stabilized as of now. The more restrictions we put on code we write in the tree in these days, the less work we will need to do to adapt the code to future spec changes.

Comment 4

[Brandon Benvie [:benvie]]

(In reply to :Paolo Amadini from comment #3)
> I have a few doubts about hurrying up too much here, maybe we should require
> Chrome consumers to live without a few features for a while. Again, feel
> free to check with Dave as I'll be away next week and I may or may not have
> Internet access.
> 
> As long as we have two Promise implementations in the tree, I'm wary about
> implementing "catch" for Chrome code. In case an Add-on SDK promise is
> handed off to code that uses "catch", things could break.

Breakage would be inssue that we that we may need to be aware of.

> With regard to "Promise.cast", what is its advantage over "Promise.resolve"?
> In fact, differently from what the name suggests, it seems that this "cast"
> implementation does not even guarantee that the returned value is of our
> desired Promise type, if a "thenable" is passed as its argument.

I implemented "cast" wrong in the patch you've seen. The actual implementation doesn't check for thenables, it checks for the existence of one of the hidden properties (AKA a promise made specifically by this library). It's an optimization, but a really useful one. My patch will be updated to fix this check.

> I realize the latter could be a specification question, but I'm not sure
> about how much the spec is stabilized as of now. The more restrictions we
> put on code we write in the tree in these days, the less work we will need
> to do to adapt the code to future spec changes.

The spec is incredibly stable for now. I'm awaiting literal proof from the author, but until then I will personally guarantee that this is what will be in ES6, to the letter.

Comment 5

[Brandon Benvie [:benvie]]

On further reflection, I agree with you. We should hold off until it lands in the ES6 spec proper. I think this will happen within the next few months, so there's no rush.

Comment 6

[:Paolo Amadini]

According to the conclusion in bug 966348, I feel that we can now go forward with this new API in Promise.jsm, with an optimized Promise.resolve that takes the place of Promise.cast.

Promise.resolve would simply return aValue when it is an instance of PromiseImpl.

We should also add Promise.prototype.catch to the Add-on SDK Promise API at the same time, to avoid the interoperability issues mentioned earlier.

Comment 7

[Brandon Benvie [:benvie]]

Attachment: promise-api.patch

Updates the patch to remove Promise.cast and update Promise.resolve to pass-through promises. Also adds tests.

Comment 8

[Brandon Benvie [:benvie]]

The patch doesn't use an instanceof check, rather (approximately) follows what the spec says to do, which is to check for the existence of promise-only metadata. Changing this to use instanceof should be fine though, since the purpose of this in the spec is to allow for subclassing, which we don't have to worry about here.

Comment 9

[Brandon Benvie [:benvie]]

Attachment: promise-api.patch

Changes Promise.resolve to use instanceof.

https://tbpl.mozilla.org/?tree=Try&rev=90dc7cff85d1

Comment 10

[Brandon Benvie [:benvie]]

Attachment: promise-api.patch

Misnamed one of the test functions.

Comment 11

[:Paolo Amadini]

Sorry for the delay here, I think the patch looks good overall, will take a closer look later.

In the meantime, can you look into adding the "catch" method to the Add-on SDK as well? I don't know how the check-in process works there, but I'd like these two changes to land as close as possible to each other, since we already have interactions between the two libraries.

Comment 12

[Brandon Benvie [:benvie]]

(In reply to :Paolo Amadini from comment #11)
> Sorry for the delay here, I think the patch looks good overall, will take a
> closer look later.

No problem! Thanks.

> In the meantime, can you look into adding the "catch" method to the Add-on
> SDK as well? I don't know how the check-in process works there, but I'd like
> these two changes to land as close as possible to each other, since we
> already have interactions between the two libraries.

Yeah, I've talked to some addon-sdk folks. This should be fine (I'll also add race for parity). The process involves a pull request on github and then once it lands there it'll get merged to m-c within a week or two, so there will be a bit of lagtime.

Comment 13

[:Paolo Amadini]

Comment on attachment 8375988 [details] [diff] [review]
promise-api.patch

Review of attachment 8375988 [details] [diff] [review]:
-----------------------------------------------------------------

There are a few open questions regarding some corner cases checked by the tests, the implementation looks good.

I think this will be an easy r+ with these fixed, though I'd like to see the Add-on SDK patch for "catch" landed on their repository before this lands. Note that there is no need to port the static functions to the SDK.

::: toolkit/modules/Promise.jsm
@@ +416,5 @@
>  };
>  
> +/**
> + * Invokes `promise.then` with undefined for the resolve handler and a given
> + * reject handler.

nit: We don't use the backwards apostrophe character in this module, double quotes can be used here:

Invokes the "then" method with the given rejection handler, passing undefined for the fulfillment handler.

@@ +421,5 @@
> + *
> + * @param aOnReject
> + *        The rejection handler.
> + *
> + * @return A new pending promise returned.

@return The promise returned by the "then" method.

@@ +546,5 @@
> + *        be resolved or rejected as to the given value or reason.
> + *
> + * @return A new promise that is fulfilled when any values are resolved or
> + *         rejected. Its resolution value will be forwarded from the resolution
> + *         value or rejection reason.

nit: We've been using the terms "resolved", "fulfilled", and "rejected" in this module in a way that is inconsistent with the latest specification. Despite that, do you think you can update the text here for a more consistent usage? It will help when porting this new piece of documentation to MDN.

::: toolkit/modules/tests/xpcshell/test_Promise.js
@@ +705,5 @@
> +  make_promise_test(function race_resolve(test) {
> +    let p1 = Promise.resolve(1);
> +    let p2 = Promise.resolve().then(() => 2);
> +
> +    return Promise.race([p1, p2]).then(

This is actually subject to race conditions :-) There is no guarantee made on the order of resolution of two different promises (only on the invocations of "then" on the same promise).

In this test, we can simply avoid resolving p2 until the promise returned by "race" is resolved.

@@ +724,5 @@
> +    }
> +
> +    return Promise.race([1, 2, 3]).then(
> +      function onResolve(value) {
> +        do_check_eq(value, 1);

Is "race" guaranteed to always return the first non-promise value in the iterable? If so, this should be documented. (And if not, an @note to that effect could be useful as well, while the test should be changed.)

@@ +743,5 @@
> +        }
> +      );
> +
> +      // Approximate "never" so we don't have to solve the halting problem.
> +      do_timeout(200, resolve);

:-)

@@ +758,5 @@
> +    }
> +
> +    return Promise.race(iterable()).then(
> +      function onResolve(value) {
> +        do_check_eq(value, 1);

ditto

@@ +792,5 @@
> +    let p1 = Promise.reject(1);
> +    let p2 = Promise.resolve(2);
> +    let p3 = Promise.resolve(3);
> +
> +    return Promise.race([p1, p2, p3]).then(

ditto with regard to passing an already rejected promise first.

In case the order is unimportant, we should delay resolution. In case the order is important, we should add the opposite test (fulfilled promise placed before rejected promise).

Comment 14

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #13)
> Note that there is no need to port the static functions to the SDK.

To clarify, I'm not saying not to do that, just that it's not a requirement for landing this patch. Feel free to port "race" if you want, though their implementation of "all" is different (it also accepts a list of arguments) and aligning it to the specification might be more effort than required now.

Comment 15

[Brandon Benvie [:benvie]]

In response to the race comments:

(In reply to :Paolo Amadini from comment #13)
> ::: toolkit/modules/tests/xpcshell/test_Promise.js
> @@ +705,5 @@
> > +  make_promise_test(function race_resolve(test) {
> > +    let p1 = Promise.resolve(1);
> > +    let p2 = Promise.resolve().then(() => 2);
> > +
> > +    return Promise.race([p1, p2]).then(
> 
> This is actually subject to race conditions :-) There is no guarantee made
> on the order of resolution of two different promises (only on the
> invocations of "then" on the same promise).

The spec says:

"The PendingTask records from a single Task Queue are always initiated in FIFO order. This specification does not define the order in which multiple Task Queues are serviced. An ECMAScript implementation may interweave the FIFO evaluation of the PendingTask records of a Task Queue with the evaluation of the PendingTask records of one or more other Task Queues."

All Promise tasks go into the same PromiseTask Queue and so are executed in FIFO order.

1.) Promise.resolve(1) -> a task will be added to the Queue
2.) Promise.resolve() -> a task will be added to the Queue
3.) microtask finishes
4.) tasks are added to the Queue -> Queue looks like [resolve(1), resolve(undefined)]
4.) new microtask, begin processing PromiseTask Queue
5.) process first task in Queue -> Queue looks like [resolve(undefined)]
6.) process next task in Queue, a new task will be added to the Queue -> Queue looks like []
7.) microtask finishes
8.) task is added to the Queue -> Queue looks like [resolve(2)]
9.) new microtask, begin processing PromiseTask Queue
10.) process first task in Queue -> Queue looks like []
11.) microtask finishes

As long as promises have a deterministic resolution time, e.g. you're resolving with primitive values, the processing order is deterministic and the result of race will be deterministic.

> @@ +724,5 @@
> > +    }
> > +
> > +    return Promise.race([1, 2, 3]).then(
> > +      function onResolve(value) {
> > +        do_check_eq(value, 1);
> 
> Is "race" guaranteed to always return the first non-promise value in the
> iterable? If so, this should be documented. (And if not, an @note to that
> effect could be useful as well, while the test should be changed.)

Same as above. It seems like we might want to document in general how Task Queues work since this goes beyond race.

Comment 16

[:Paolo Amadini]

(In reply to Brandon Benvie [:benvie] from comment #15)
> 1.) Promise.resolve(1) -> a task will be added to the Queue
> 2.) Promise.resolve() -> a task will be added to the Queue
> 3.) microtask finishes
> 4.) tasks are added to the Queue -> Queue looks like [resolve(1),
> resolve(undefined)]
> 4.) new microtask, begin processing PromiseTask Queue
> 5.) process first task in Queue -> Queue looks like [resolve(undefined)]
> 6.) process next task in Queue, a new task will be added to the Queue ->
> Queue looks like []
> 7.) microtask finishes
> 8.) task is added to the Queue -> Queue looks like [resolve(2)]
> 9.) new microtask, begin processing PromiseTask Queue
> 10.) process first task in Queue -> Queue looks like []
> 11.) microtask finishes
> 
> As long as promises have a deterministic resolution time, e.g. you're
> resolving with primitive values, the processing order is deterministic and
> the result of race will be deterministic.

So, I think a simplified version of this list of events should be added as a comment inside the test itself. This makes it clear that the particular usage pattern only makes sense in a controlled environment like the xpcshell test.

> > Is "race" guaranteed to always return the first non-promise value in the
> > iterable? If so, this should be documented. (And if not, an @note to that
> > effect could be useful as well, while the test should be changed.)
> 
> Same as above. It seems like we might want to document in general how Task
> Queues work since this goes beyond race.

Given the above, I'd agree we shouldn't document the "race" function itself, in particular in a way that suggests that the caller may rely on any particular behavior. Again, we might want to document the test in the opposite direction, making it clear that the behavior is deterministic there, but ideally not to be relied upon.

Comment 17

[:Paolo Amadini]

Comment on attachment 8375988 [details] [diff] [review]
promise-api.patch

Brandon, given that bug 988122 will add general availability of Promise objects having the "catch" method, I think our best option is to land this now, even before "catch" is added to SDK Promises.

Upgrading the patch to r+ since functionally it looks good after your replies to my previous comments. If you feel like addressing the nits, feel free to do so, otherwise I'll land the original version in the following days.

Comment 18

[:Paolo Amadini]

Attachment: Brandon's patch updated

https://tbpl.mozilla.org/?tree=Try&rev=d1731d0984d6

Comment 19

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/dc04e14d954b

Comment 20

[Brandon Benvie [:benvie]]

(In reply to :Paolo Amadini from comment #17)
> Comment on attachment 8375988 [details] [diff] [review]
> promise-api.patch
> 
> Brandon, given that bug 988122 will add general availability of Promise
> objects having the "catch" method, I think our best option is to land this
> now, even before "catch" is added to SDK Promises.
> 
> Upgrading the patch to r+ since functionally it looks good after your
> replies to my previous comments. If you feel like addressing the nits, feel
> free to do so, otherwise I'll land the original version in the following
> days.

With this landed, it would be good to do a full pass updating the comments to use the spec terminology for fulfilled/resolved/rejected and to also to comment the way the Task Queue works, since it is implicit in our implementation.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dc04e14d954b