Created attachment 152576 [details]
Promise abstraction

Created attachment 152577 [details]
Async Function abstraction

Created attachment 152578 [details]
test cases for promise to illustrate use

Created attachment 152579 [details]
test cases for async func to illustrate use

can you give a real example of using, say, a dbus API? in the test programs it's hard to see what's really part of the idiom and what's just part of the test framework.

well, we haven't moved dbus.js over to this

But a couple of examples.

One case it cleans up is if you have a chain of async calls that depend on each other. Say you have:

function getA(onReturn, onError) { ... }
function getB(onReturn, onError, valueFromA) { ... }
function getC(onReturn, onError, valueFromB) { ... }

so the "old way" is something like:

getA(function() {
    getB(function(vA) { 
       getC(function(vB) { 
       }
    }
}

(only a lot more cluttered in real life, and optionally using Lang.bind)

with the fromGenerator stuff in the new way, you would write this roughly as:

fromGenerator(function() {
  let vA = yield getA();
  let vB = yield getB(vA);
  let vC = yield getC(vB);
});

each "yield" is saving the state of the generator function, passing flow control back up to the main loop which then passes control back to the generator function once the promise has its value.

Error handling is a big virtue of this approach, since if any of those async calls would "onError" then the "yield" will simply throw and the other async calls don't end up running.

You can imagine that getA, getB, getC are dbus calls and see how this would apply to dbus.

Promise also allows you to have multiple callbacks waiting on the same value (multiple calls to Promise.get are allowed) which can be pretty useful.

There are various other cases where you can use Promise to clean things up.

For example if you have code where you want to wait on multiple async results to all arrive, then continue, you can nicely abstract that.

Another aspect of this is just standardizing the async convention so people aren't making up lots of different ways to do the callbacks.

I guess none of these are very concrete or good examples; everything in our code is fairly complex and not a great example to just cut-and-paste so I'm struggling to pick one.

A really good example, though I don't know if Telepathy actually works this way, would be something like Telepathy; say you want to do the following sequence of things:

* get list of accounts
 * for each account (in parallel)
   * get list of buddies
      * for each buddy (in parallel)
        * get name and avatar

If you try to write a function that returns the list of accounts, with all buddies, buddy names, and avatars already filled in, using current dbus callbacks setup, all async, then you'll have a pretty ugly mess on your hands. But with the abstraction in these files it's reasonably manageable.

Created attachment 162942 [details] [review]
[modules] Add a promise module

Promise is a better API for supporting asynchronous code.

Created attachment 162944 [details] [review]
[modules] Add a promise module

Promise is a better API for supporting asynchronous code.

This is what I just pushed to masterr, the tests and highlevel API
still remains to be discussed & reviewed.

There's also the Deferred model used by Twisted (and Dojo/MochiKit/Clojure). The Telepathy example (assuming an inlineCallbacks-like strategy) would be:

    function getAllBuddyNamesAndAvatars(tp) {
        let namesAndAvatars = {};
        let accounts = yield tp.get_accounts();
        for (let i = 0; i < accounts.length; i ++) {
            let account = accounts[i];
            let buddies = yield account.get_buddies();
            for (let j = 0; j < buddies.length; j ++) {
                let buddy = buddies[j];
                namesAndAvatar[buddy.get_id()] = buddy.get_name_and_avatar();
            }
        }
        returnValue(namesAndAvatars);
    }

Unfortunately, the problem with this solution is that it's no better than the blocking solution: we're waiting on getting the first account's buddies to return before we can even query for the second account's buddies. It's still "blocking code" except that we don't block the mainloop. Something which queries for the buddies individually would still have to use callbacks:

    function getAllBuddyNamesAndAvatars(tp) {
        let namesAndAvatars = {};
        let accounts = yield tp.get_accounts();
        for (let i = 0; i < accounts.length; i ++) {
            let account = accounts[i];
            let d = accounts.get_buddies();
            d.addCallback(function(buddies) {
                for (let j = 0; j < buddies.length; j ++) {
                    let buddy = buddies[j];
                    let d2 = buddy.get_name_and_avatar();
                    d2.addCallback(function(nameAndAvatar) {
                        namesAndAvatars[buddy.get_id()] = nameAndAvatar;
                    });
                }
            });
        }
        returnValue(namesAndAvatars);
    }

Effectively, all of the "async calls" return a Deferred. Due to how generators work in JS (and Python), yielding will pause execution of the generator until it's resumed with a "g.send(result)". If we yield a Deferred, the "inlineCallbacks" wrapper attaches a callback to the yielded Deferred, resuming execution of the generator when done. If we don't yield a generator, we just get it back plainly as the result of the function. We can then attach our own callback to it, which doesn't block anything. So, we get the best of both worlds: sometimes, we can write what looks like blocking code when we're waiting on a single result, and we can wait on multiple things. Oh, and if we ever need to wait until multiple results finish, but they can happen independently:

    function getAllWebsites() {
        let d1 = getPage("http://google.com");
        let d2 = getPage("http://wikipedia.org");
        let d3 = getPage("http://redhat.com");

        let dl = DeferredList([d1, d2, d3]);
        dl.addCallback(function(results) {
             let [google, wiki, rh] = results;
             // do stuff with contents of websites
        });
    }

Man, this is a lot of text. I should probably write a blog article about this or something.

I'd like to resurrect this bug. To me the question of what async paradigm to pick has been settled since the discussion 5 years ago by the community, and the answer would be ES6 promises.

Now that we are updating to SpiderMonkey 31, I sadly discovered that Promises, although they were added to Firefox 31, are not part of SpiderMonkey, because they are part of the DOM implementation. (That makes sense, because they require DOM facilities like Workers or setImmediate or whatever they use.)

I took a look at it, and it looks like it wouldn't be impossible to pull it into GJS, using GLib to replace the DOM workers:
https://dxr.mozilla.org/mozilla-esr31/source/dom/promise/Promise.cpp

However, there is a lot of Firefox-specific code to untangle that I'm unfamiliar with.

We could also pull in one of the many Javascript implementations of ES6 promises from Node.js. Bluebird seems to be the most performant, but it's also quite large and tied to Node. I would suggest Lie, which is 300 lines but reasonably fast.

I asked around in #jsapi and it looks like starting with SpiderMonkey 52, Promises are moved out of the DOM and into SpiderMonkey proper; this was necessary in order to support the async and await statements. We'll have to provide callbacks using JS::SetEnqueuePromiseJobCallback(), which we can quite easily implement using GLib.

Given that any solution I add now will be temporary, I'm inclined to go with the pure-JS implementation, and jettison it later in favour of built-in Promises.

Created attachment 340090 [details] [review]
promise: Add Lie, an ES6 promises implementation

This doesn't work as-is, but I'm putting the original code in a separate
commit so that it's clear later what modifications were made.

This is version 3.1.0 of Lie.

Created attachment 340091 [details] [review]
promise: Get node-isms to work

This adds some stubs to lie.js for node.js-related objects that it
expects to be present, such as "process".

It implements "require" as well, and provides an implementation of
immediate.js using GLib. It would have been somewhat feasible to port
immediate.js, but it requires either a DOM or setTimeout(), so we would
have had to basically write this implementation anyway.

It also wraps the whole thing in an IIFE so that symbols are not leaked
out of the scope, and returns the Promise object so that it will be the
result of calling JS::Evaluate() on the code.

Created attachment 340092 [details] [review]
promise: Fix SpiderMonkey strict mode warnings

By default, our JS engine warns a bit overzealously about functions that
don't always return a value; fix this by moving some return statements
around. It also warns about 'use strict' used elsewhere than the very
beginning of a function or file, so fix that as well.

(You can turn off the extra warnings with GJS_DISABLE_EXTRA_WARNINGS, but
we don't want the user to see extra warnings from the Promise code even
if extra warnings are turned on. We can't disable the option from code,
as it is set per context.)

We annotate all modifications to the original Lie code with comments, so
it's easy to tell what to modify if we ever wanted to pull in a new
version.

Created attachment 340093 [details] [review]
context: Define global Promise object

This imports the _lie module automatically and installs it on our global
object as the Promise object.

Created attachment 340094 [details] [review]
WIP - Gio.wrapPromise()

This is a wrapper to convert GIO-style async functions into promises.

Gio.wrapPromise(obj, 'method_async', 'method_finish', ...)

returns a promise that is fulfilled when the operation finishes
successfully and rejected when the operation finishes unsuccessfully or
is cancelled. For ..., pass additional in-arguments to the async method,
but leave out the Gio.Cancellable() - the wrapper creates one
automatically.

It adds a cancel() method to the returned promise as well, even though
ES6 doesn't define one. I'm not sure if it's a good idea.

Another thing to consider is I/O priority; it would be cool to use the
passed-in I/O priority as the priority for our implementation of
immediate().

I'm not a big fan of passing the method names like this. It would be
nicer if we could discover a bit more automatically, perhaps by using GI.
Particularly the finish method would be nice to figure out automatically.

I'd like to include the tests from [1] and [2] as well, but I need to wait until I've ported the test suite to use an embedded copy of Jasmine; I'm not too excited about porting all those tests in `describe`/`it` format to JSUnit.

[1] https://github.com/promises-es6/promises-es6
[2] https://github.com/promises-aplus/promises-tests

Review of attachment 340090 [details] [review]:

Not going to review this line-by-line of course, so looks good.

Review of attachment 340091 [details] [review]:

OK

Review of attachment 340092 [details] [review]:

OK

Review of attachment 340093 [details] [review]:

OK

Attachment 340090 [details] pushed as 8773579 - promise: Add Lie, an ES6 promises implementation
Attachment 340091 [details] pushed as 4c326a5 - promise: Get node-isms to work
Attachment 340092 [details] pushed as 8977798 - promise: Fix SpiderMonkey strict mode warnings
Attachment 340093 [details] pushed as 9e3220e - context: Define global Promise object

Created attachment 341996 [details] [review]
promise: Fix missing braces

Caught by benwaffle on IRC

Review of attachment 341996 [details] [review]:

Good catch!

Just pushed another patch adding .setSourceIsLazy(true) to the compile
options for the promise library. Without it, GJS would crash on exit on my
other machine, I suspect because the interpreter would cache the compiled
source but we freed the original source string.

Comment on attachment 341996 [details] [review]
promise: Fix missing braces

Attachment 341996 [details] pushed as 9746325 - promise: Fix missing braces

Created attachment 343614 [details] [review]
promise: Add test suites

This adds existing promises test suites from [1] and [2], with some
adapter code to convert the very similar Mocha test framework code to
work with our built-in Jasmine. We do skip a few of the promises-aplus
tests because Jasmine doesn't currently support querying the order in
which spies were called.

[1] https://github.com/promises-aplus/promises-tests
[2] https://github.com/promises-es6/promises-es6

Created attachment 343615 [details] [review]
promise: Fix ES6 compatibility

The promises-es6 tests mandate a few checks on the input argument to "new
Promise()".

Created attachment 343616 [details] [review]
promise: Skip a few promises-es6 tests

These I'm pretty sure are not compatible with Lie. They are fairly minor
tests stipulating that errors should be thrown when a promise is
subclassed incorrectly.

I'm kind of ambivalent about committing that entire test suite. It bloats the unit tests (adding 1000 new tests) and once Promises are in SpiderMonkey I'd generally want to refrain from testing them. Maybe having run our promise library against these tests once is enough?

Comment on attachment 162944 [details] [review]
[modules] Add a promise module

This was committed a long time ago and has since even been removed.

Comment on attachment 340094 [details] [review]
WIP - Gio.wrapPromise()

I'm going to split the GIO wrapper off into its own bug, because it's likely to land at an entirely different pace than the rest of this.

Review of attachment 343615 [details] [review]:

OK

Review of attachment 343614 [details] [review]:

I am not sure it's worth to have the whole test suite to be honest, because eventually the promise module will be provided directly by mozjs. I will leave it up to you, but I am leaning towards not having it.

Review of attachment 343616 [details] [review]:

If we commit the tests, feel free to push this too.

Once I had the whole test suite running and saw that it added like 1500 tests, I started having second thoughts about it as well, especially because as you point out it is not even permanent.

I think we can close this bug, although when we do get native mozjs promises I'd like to dig up that patch again and run the tests, but still we don't need to commit it.

Attachment 343615 [details] pushed as ee99b27 - promise: Fix ES6 compatibility