Harmony promises

Harmony promises

Based on prototype at

  https://github.com/rossberg-chromium/js-promise

which informed the latest spec draft version at

  https://github.com/domenic/promises-unwrapping/blob/master/README.md

Activated by --harmony-promises.

Feature complete with respect to the draft spec, plus the addition of .when and .deferred methods. Final naming and other possible deviations from the current draft will hopefully be resolved soon after the next TC39 meeting.

This CL also generalises the Object.observe delivery loop into a simplistic microtask loop. Currently, all observer events are delivered before invoking any promise handler in a single fixpoint iteration. It's not clear yet what the final semantics is supposed to be (should there be a global event ordering?), but it will probably require a more thorough event loop abstraction inside V8 once we get there.

R=dslomov@chromium.org, yhirano@chromium.org
BUG=

Committed: http://code.google.com/p/v8/source/detail?r=18113

[rafaelw, 2013-11-14 21:08:46]

https://codereview.chromium.org/64223010/diff/1/src/execution.h
File src/execution.h (right):

https://codereview.chromium.org/64223010/diff/1/src/execution.h#newcode175
src/execution.h:175: static void RunMicrotasks(Isolate* isolate);
ExecuteAllMicrotaskWork?

https://codereview.chromium.org/64223010/diff/1/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/64223010/diff/1/src/object-observe.js#newcode387
src/object-observe.js:387: %SetMicrotasksPending(true);
nit: All of these work items are run within a single "microtask". Thus would
better be named: SetMicrotaskWorkPending

https://codereview.chromium.org/64223010/diff/1/src/object-observe.js#newcode565
src/object-observe.js:565: function ObserveMicrotasksRunner() {
nit: along the same lines: ObserveMicrotaskRunner (singular)

[Michael Starzinger, 2013-11-15 09:37:19]

Small drive-by.

https://codereview.chromium.org/64223010/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/64223010/diff/1/src/runtime.cc#newcode14621
src/runtime.cc:14621: RUNTIME_FUNCTION(MaybeObject*, Runtime_FailHard) {
This is similar to the existing Runtime_Abort, can we name this one similarly?
Something like Runtime_AbortJS maybe.

https://codereview.chromium.org/64223010/diff/1/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/64223010/diff/1/src/runtime.h#newcode358
src/runtime.h:358: F(FailHard, 1, 1) \
Let's move this declaration near Runtime_Abort.

[rossberg, 2013-11-15 09:58:46]

On 2013/11/14 21:08:46, rafaelw wrote:
> nit: All of these work items are run within a single "microtask". Thus would
> better be named: SetMicrotaskWorkPending

Hm, OK, now I am confused about the terminology; I suppose my DOM ignorance is
showing. :) What's the definition of 'microtask'? And what is the official term
for the run of a single event handler in this context?

[rossberg, 2013-11-15 10:00:57]

https://codereview.chromium.org/64223010/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/64223010/diff/1/src/runtime.cc#newcode14621
src/runtime.cc:14621: RUNTIME_FUNCTION(MaybeObject*, Runtime_FailHard) {
On 2013/11/15 09:37:20, Michael Starzinger wrote:
> This is similar to the existing Runtime_Abort, can we name this one similarly?
> Something like Runtime_AbortJS maybe.

Done.

https://codereview.chromium.org/64223010/diff/1/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/64223010/diff/1/src/runtime.h#newcode358
src/runtime.h:358: F(FailHard, 1, 1) \
On 2013/11/15 09:37:20, Michael Starzinger wrote:
> Let's move this declaration near Runtime_Abort.

Done.

[esprehn, 2013-11-15 11:23:19]

https://codereview.chromium.org/64223010/diff/120001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/120001/src/promise.js#newcode191
src/promise.js:191: try { deferred.reject(e) } catch(e) {}
Swallowing exceptions like this is bad for developers, it should get logged to
the console or something.

[rossberg, 2013-11-15 12:56:56]

https://codereview.chromium.org/64223010/diff/120001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/120001/src/promise.js#newcode191
src/promise.js:191: try { deferred.reject(e) } catch(e) {}
On 2013/11/15 11:23:19, esprehn wrote:
> Swallowing exceptions like this is bad for developers, it should get logged to
> the console or something.

Good point, although we currently do the same for Object.observe observers. I'll
make that a TODO, as we still lack a good infrastructure for a better solution.

[yhirano, 2013-11-18 03:59:04]

https://codereview.chromium.org/64223010/diff/210001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/210001/src/promise.js#newcode248
src/promise.js:248: function PromiseAll(values) {
This function returns a promise which will never be resolved nor rejected, but
the behavior is not correct.

https://codereview.chromium.org/64223010/diff/210001/src/promise.js#newcode283
src/promise.js:283: "one", PromiseOne,
Is this Promise.race?

https://codereview.chromium.org/64223010/diff/210001/src/promise.js#newcode287
src/promise.js:287: "when", PromiseWhen,
There is no Promise.prototype.when in the spec draft.

[yhirano, 2013-11-18 04:01:31]

https://codereview.chromium.org/64223010/diff/210001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/210001/src/promise.js#newcode287
src/promise.js:287: "when", PromiseWhen,
On 2013/11/18 03:59:04, yhirano wrote:
> There is no Promise.prototype.when in the spec draft.

Oh, I didn't read the description. Never mind.

[rossberg, 2013-11-18 10:56:04]

https://codereview.chromium.org/64223010/diff/210001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/210001/src/promise.js#newcode248
src/promise.js:248: function PromiseAll(values) {
On 2013/11/18 03:59:04, yhirano wrote:
> This function returns a promise which will never be resolved nor rejected, but
> the behavior is not correct.

Oops, I somehow managed to misread the spec 8-}, and simply resolved the promise
to 'undefined' once all are resolved. Fixed.

https://codereview.chromium.org/64223010/diff/210001/src/promise.js#newcode283
src/promise.js:283: "one", PromiseOne,
On 2013/11/18 03:59:04, yhirano wrote:
> Is this Promise.race?

Yes, added a comment.

[yhirano, 2013-11-18 11:03:45]

https://codereview.chromium.org/64223010/diff/210001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/210001/src/promise.js#newcode248
src/promise.js:248: function PromiseAll(values) {
Sorry, my comment was wrong.
I wanted to say:

This function returns a promise which will never be resolved nor rejected **when
values is an empty array**, but
the behavior is not correct.

[rossberg, 2013-11-18 11:51:09]

https://codereview.chromium.org/64223010/diff/210001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/210001/src/promise.js#newcode248
src/promise.js:248: function PromiseAll(values) {
On 2013/11/18 11:03:46, yhirano wrote:
> Sorry, my comment was wrong.
> I wanted to say:
> 
> This function returns a promise which will never be resolved nor rejected
**when
> values is an empty array**, but
> the behavior is not correct.

Ah, excellent point. Fixed and test case added (also for Promise.one([])).

[yhirano, 2013-11-19 03:14:06]

https://codereview.chromium.org/64223010/diff/460001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode66
src/promise.js:66: resolver(function(x) { PromiseResolve(promise, x) },
Should throw a TypeError if resolver is not callable.

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode66
src/promise.js:66: resolver(function(x) { PromiseResolve(promise, x) },
Should reject |this| when an exception is thrown by resolver.

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode89
src/promise.js:89: function PromiseResolve(promise, x) {
Since a thenable may call PromiseResolve and PromiseReject multiple times in
arbitrary order, I think we should do nothing if promise is not pending.

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode93
src/promise.js:93: function PromiseReject(promise, r) {
ditto

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode222
src/promise.js:222: } else if (!IS_NULL_OR_UNDEFINED(x) && 'then' in
TO_OBJECT_INLINE(x)) {
and x.then is callable

[rossberg, 2013-11-19 12:28:42]

https://codereview.chromium.org/64223010/diff/460001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode66
src/promise.js:66: resolver(function(x) { PromiseResolve(promise, x) },
On 2013/11/19 03:14:07, yhirano wrote:
> Should throw a TypeError if resolver is not callable.

It does. ;)

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode66
src/promise.js:66: resolver(function(x) { PromiseResolve(promise, x) },
On 2013/11/19 03:14:07, yhirano wrote:
> Should reject |this| when an exception is thrown by resolver.

That deviation is intentional: turning a synchronous error into asynchronous
seems counterproductive. I intend to propose a change.

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode89
src/promise.js:89: function PromiseResolve(promise, x) {
On 2013/11/19 03:14:07, yhirano wrote:
> Since a thenable may call PromiseResolve and PromiseReject multiple times in
> arbitrary order, I think we should do nothing if promise is not pending.

I had hoped for the possibility of a less sloppy semantics, but compatibility
with legacy code might indeed be a problem.

Done.

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode93
src/promise.js:93: function PromiseReject(promise, r) {
On 2013/11/19 03:14:07, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode222
src/promise.js:222: } else if (!IS_NULL_OR_UNDEFINED(x) && 'then' in
TO_OBJECT_INLINE(x)) {
On 2013/11/19 03:14:07, yhirano wrote:
> and x.then is callable

Done.

[yhirano, 2013-11-20 07:05:36]

https://codereview.chromium.org/64223010/diff/460001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode66
src/promise.js:66: resolver(function(x) { PromiseResolve(promise, x) },
On 2013/11/19 12:28:43, rossberg wrote:
> On 2013/11/19 03:14:07, yhirano wrote:
> > Should throw a TypeError if resolver is not callable.
> 
> It does. ;)
Sorry, you are right.

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode66
src/promise.js:66: resolver(function(x) { PromiseResolve(promise, x) },
On 2013/11/19 12:28:43, rossberg wrote:
> On 2013/11/19 03:14:07, yhirano wrote:
> > Should reject |this| when an exception is thrown by resolver.
> 
> That deviation is intentional: turning a synchronous error into asynchronous
> seems counterproductive. I intend to propose a change.
Then please add comments.

https://codereview.chromium.org/64223010/diff/560001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/560001/src/promise.js#newcode185
src/promise.js:185: else if (IsPromise(result))
I think statements here correspond to UpdateDeferredFromPotentialThenable in the
spec draft, but slightly differ. Can you explain?

[rossberg, 2013-11-22 11:03:06]

https://codereview.chromium.org/64223010/diff/460001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/460001/src/promise.js#newcode66
src/promise.js:66: resolver(function(x) { PromiseResolve(promise, x) },
On 2013/11/20 07:05:36, yhirano wrote:
> On 2013/11/19 12:28:43, rossberg wrote:
> > On 2013/11/19 03:14:07, yhirano wrote:
> > > Should reject |this| when an exception is thrown by resolver.
> > 
> > That deviation is intentional: turning a synchronous error into asynchronous
> > seems counterproductive. I intend to propose a change.
> Then please add comments.

Done.

https://codereview.chromium.org/64223010/diff/560001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/560001/src/promise.js#newcode185
src/promise.js:185: else if (IsPromise(result))
On 2013/11/20 07:05:36, yhirano wrote:
> I think statements here correspond to UpdateDeferredFromPotentialThenable in
the
> spec draft, but slightly differ. Can you explain?

That's because the spec currently does not separate chaining from recursive
unwrapping and thenable coercion. This implementation factors the latter out
into the wrapped resolution handler constructed in the .then method, so can
simply call .when here.

(Also, I don't understand what happens under step 3 of this algorithm -- that
just seems wrong, I filed a bug.)

[yhirano, 2013-11-25 01:31:14]

lgtm

[rossberg, 2013-11-26 13:23:52]

https://codereview.chromium.org/64223010/diff/1/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/64223010/diff/1/src/object-observe.js#newcode387
src/object-observe.js:387: %SetMicrotasksPending(true);
On 2013/11/14 21:08:46, rafaelw wrote:
> nit: All of these work items are run within a single "microtask". Thus would
> better be named: SetMicrotaskWorkPending

Done.

https://codereview.chromium.org/64223010/diff/1/src/object-observe.js#newcode565
src/object-observe.js:565: function ObserveMicrotasksRunner() {
On 2013/11/14 21:08:46, rafaelw wrote:
> nit: along the same lines: ObserveMicrotaskRunner (singular)

Done.

[Dmitry Lomov (no reviews), 2013-11-27 12:39:48]

Very nice! I only have questions around implementation of PromiseAll

https://codereview.chromium.org/64223010/diff/880001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode256
src/promise.js:256: ++count;
I am not sure I understand this algorithm very well.

https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode260
src/promise.js:260: if (--count === 0) deferred.resolve(resolutions);
If values[0] are already resolved, won't this cause immediate resolution of
resolved, even though other values are not resolved yet?

https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode263
src/promise.js:263: if (count > 0) { count = 0; deferred.reject(r) }
why 'íf (count > 0)' condition here? If we got here, count is guaranteed to be >
0, isn't it?

https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode267
src/promise.js:267: if (count === 0) deferred.resolve(resolutions);
This is only useful when values.length === 0, right?

Suggest rewriting this by:
1) setting the count to values.length in the beginning and not incrementing in
the loop
2) moving this 'if' to before the loop.
That would make the algorithm clearer I think.

https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/ite...
File test/mjsunit/harmony/iteration-semantics.js (right):

https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/ite...
test/mjsunit/harmony/iteration-semantics.js:29: // Flags: --harmony-generators
--harmony-scoping --harmony-proxies
why this needs --harmony-proxies?

https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/pro...
File test/mjsunit/harmony/promises.js (right):

https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/pro...
test/mjsunit/harmony/promises.js:36: print(s, "succeeded")
nit: our tests do not usually print.

[rossberg, 2013-11-27 15:23:49]

[I repeatedly got errors from AppEngine uploading my latest patch, then it
showed up twice, then I tried to manually delete the duplicates, deleted one
patch too many, and now the patch with your comments is gone. Sorry about that!
I'll reply here.]

On 2013/11/27 12:39:48, Dmitry Lomov (chromium) wrote:
> Very nice! I only have questions around implementation of PromiseAll
> 
> https://codereview.chromium.org/64223010/diff/880001/src/promise.js
> File src/promise.js (right):
> 
> https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode256
> src/promise.js:256: ++count;
> I am not sure I understand this algorithm very well.
> 
> https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode260
> src/promise.js:260: if (--count === 0) deferred.resolve(resolutions);
> If values[0] are already resolved, won't this cause immediate resolution of
> resolved, even though other values are not resolved yet?

Not sure I understand the question -- 'count' counts the number of values still
awaiting resolution, and once it drops to 0 we can resolve the result.

> https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode263
> src/promise.js:263: if (count > 0) { count = 0; deferred.reject(r) }
> why 'íf (count > 0)' condition here? If we got here, count is guaranteed to be
>
> 0, isn't it?

Not if there are multiple failures. This is just a way to avoid calling
deferred.reject/resolve multiple times.

> https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode267
> src/promise.js:267: if (count === 0) deferred.resolve(resolutions);
> This is only useful when values.length === 0, right?
> 
> Suggest rewriting this by:
> 1) setting the count to values.length in the beginning and not incrementing in
> the loop
> 2) moving this 'if' to before the loop.
> That would make the algorithm clearer I think.

Done.


>
https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/ite...
> File test/mjsunit/harmony/iteration-semantics.js (right):
> 
>
https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/ite...
> test/mjsunit/harmony/iteration-semantics.js:29: // Flags: --harmony-generators
> --harmony-scoping --harmony-proxies
> why this needs --harmony-proxies?

Because there are some tests with proxies as iterators in the end.


>
https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/pro...
> File test/mjsunit/harmony/promises.js (right):
> 
>
https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/pro...
> test/mjsunit/harmony/promises.js:36: print(s, "succeeded")
> nit: our tests do not usually print.

Some do, I believe. But in any case, I needed the extra feedback here, because
if none of the async stuff actually runs at all (e.g. because something is
fundamentally broken), then you won't ever get an error either. So at least for
debugging, you want the extra assurance that something actually happened.

[Dmitry Lomov (no reviews), 2013-11-27 17:03:13]

LGTM


On 2013/11/27 15:23:49, rossberg wrote:

> On 2013/11/27 12:39:48, Dmitry Lomov (chromium) wrote:

> >
https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode260
> > src/promise.js:260: if (--count === 0) deferred.resolve(resolutions);
> > If values[0] are already resolved, won't this cause immediate resolution of
> > resolved, even though other values are not resolved yet?
> 
> Not sure I understand the question -- 'count' counts the number of values
still
> awaiting resolution, and once it drops to 0 we can resolve the result.

Sorry for being unclear - AFAIU the previous version of this algorithm would
immediately resolve the deferred if promise values[0] was already resolved (in
that case the onResolved handler would immediately get executed, the count would
be one and decremented to 0). But all is well in the new version.
> 
> >
https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode263
> > src/promise.js:263: if (count > 0) { count = 0; deferred.reject(r) }
> > why 'íf (count > 0)' condition here? If we got here, count is guaranteed to
be
> >
> > 0, isn't it?
> 
> Not if there are multiple failures. This is just a way to avoid calling
> deferred.reject/resolve multiple times.
> 
> >
https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode267
> > src/promise.js:267: if (count === 0) deferred.resolve(resolutions);
> > This is only useful when values.length === 0, right?
> > 
> > Suggest rewriting this by:
> > 1) setting the count to values.length in the beginning and not incrementing
in
> > the loop
> > 2) moving this 'if' to before the loop.
> > That would make the algorithm clearer I think.
> 
> Done.
> 
> 
> >
>
https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/ite...
> > File test/mjsunit/harmony/iteration-semantics.js (right):
> > 
> >
>
https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/ite...
> > test/mjsunit/harmony/iteration-semantics.js:29: // Flags:
--harmony-generators
> > --harmony-scoping --harmony-proxies
> > why this needs --harmony-proxies?
> 
> Because there are some tests with proxies as iterators in the end.
> 
> 
> >
>
https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/pro...
> > File test/mjsunit/harmony/promises.js (right):
> > 
> >
>
https://codereview.chromium.org/64223010/diff/880001/test/mjsunit/harmony/pro...
> > test/mjsunit/harmony/promises.js:36: print(s, "succeeded")
> > nit: our tests do not usually print.
> 
> Some do, I believe. But in any case, I needed the extra feedback here, because
> if none of the async stuff actually runs at all (e.g. because something is
> fundamentally broken), then you won't ever get an error either. So at least
for
> debugging, you want the extra assurance that something actually happened.

[rossberg, 2013-11-27 17:13:11]

On 2013/11/27 17:03:13, Dmitry Lomov (chromium) wrote:
> https://codereview.chromium.org/64223010/diff/880001/src/promise.js#newcode260
> > > src/promise.js:260: if (--count === 0) deferred.resolve(resolutions);
> > > If values[0] are already resolved, won't this cause immediate resolution
of
> > > resolved, even though other values are not resolved yet?
> > 
> > Not sure I understand the question -- 'count' counts the number of values
> still
> > awaiting resolution, and once it drops to 0 we can resolve the result.
> 
> Sorry for being unclear - AFAIU the previous version of this algorithm would
> immediately resolve the deferred if promise values[0] was already resolved (in
> that case the onResolved handler would immediately get executed, the count
would
> be one and decremented to 0). But all is well in the new version.

Ah, now I see what you mean. That wouldn't actually have happened in the old
version either, since even if value[0] was already resolved, the new handler
attached to it would not get executed immediately, but still be scheduled
asynchronously.

[rossberg, 2013-11-27 17:21:57]

Committed patchset #10 manually as r18113 (presubmit successful).