974065 - Add async function helpers

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brandon Benvie [:benvie]]

There's some simple helpers that can help boilerplate for our code that uses Task.jsm.

Comment 1

[Brandon Benvie [:benvie]]

Attachment: async-utils.patch

Adds AsyncUtils.js[m]. I'm not particularly in love with the naming of anything in this patch, so feel free to suggest names if you have better ideas.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8377909 [details] [diff] [review]
async-utils.patch

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

Let me know if there is an actual reason for the js/jsm split (I forget why DevToolsUtils did that, but I remember it was a special case). Prefer some kind of UMD boilerplate to two files if we need to support multiple envs or whatever.

::: toolkit/devtools/AsyncUtils.js
@@ +40,5 @@
> + *        The generator function that to wrap as an async function.
> + * @return Function
> + *         The async function.
> + */
> +this.asyncOnce = function asyncOnce(name, func) {

asyncOnceAndCache? asyncCallAndCache?

Do we need to take a name argument? Can't we use a gensym (maybe *optional* second param for debug)? People shouldn't be referring to the cached values directly, but calling this again and getting the cached version in an opaque-to-the-caller manner.

@@ +67,5 @@
> + *         argument, it is used as the resolution value. If there's multiple
> + *         arguments, an array containing the arguments is the resolution value.
> + *         If the method throws, the promise is rejected with the thrown value.
> + */
> +function execute(obj, func, args) {

promisify? promiseCall?

@@ +82,5 @@
> + * promise for the result.
> + *
> + * @see execute
> + */
> +this.invoke = function invoke(obj, func, ...args) {

Do we really need invoke AND execute? Can't we just choose one?

::: toolkit/devtools/AsyncUtils.jsm
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Why are we doing the DevToolsUtils double file thing for this file? Can it be simplified to be a single file and required via our loader only?

::: toolkit/devtools/tests/unit/head_devtools.js
@@ +4,5 @@
>  const Cu = Components.utils;
>  const Cr = Components.results;
>  
>  Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
> +Cu.import("resource://gre/modules/devtools/AsyncUtils.jsm");

No more jsm! :(

Let's use the loader.

::: toolkit/devtools/tests/unit/test_async.js
@@ +1,5 @@
> +/* -*- Mode: js; js-indent-level: 2; -*- */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Test AsyncUtils.safeErrorString

orly?

Comment 3

[Brandon Benvie [:benvie]]

(In reply to Nick Fitzgerald [:fitzgen] [Ðoge:D6jomNp59N9TVfgc538HU3RswwmwZwcrd7] from comment #2)
> Let me know if there is an actual reason for the js/jsm split (I forget why
> DevToolsUtils did that, but I remember it was a special case). Prefer some
> kind of UMD boilerplate to two files if we need to support multiple envs or
> whatever.

Changed this to just be a module that can be required, and changed to a single file (async-utils.js).


> ::: toolkit/devtools/AsyncUtils.js
> @@ +40,5 @@
> > + *        The generator function that to wrap as an async function.
> > + * @return Function
> > + *         The async function.
> > + */
> > +this.asyncOnce = function asyncOnce(name, func) {
> 
> asyncOnceAndCache? asyncCallAndCache?

Not sold on either of these, they don't seem to really add much explanatory power. A minor consideration is that I'd like to keep the names somewhat short since these will be used on the same line as function heads.


> Do we need to take a name argument? Can't we use a gensym (maybe *optional*
> second param for debug)? People shouldn't be referring to the cached values
> directly, but calling this again and getting the cached version in an
> opaque-to-the-caller manner.

If we were to do this we might as well use a WeakMap. I originally allowed using a name because the code that this helper is intended to work with uses that property elsewhere as an indication of state. However, after thinking about it, it would be better if this code was changed to just explicitly set a property in the async function to indicate the state.


> @@ +67,5 @@
> > + *         argument, it is used as the resolution value. If there's multiple
> > + *         arguments, an array containing the arguments is the resolution value.
> > + *         If the method throws, the promise is rejected with the thrown value.
> > + */
> > +function execute(obj, func, args) {
> 
> promisify? promiseCall?

I like promisify here. It's an internal function (not exported). I also opted to change invoke and call to promiseInvoke and promiseCall, respectively.


> 
> @@ +82,5 @@
> > + * promise for the result.
> > + *
> > + * @see execute
> > + */
> > +this.invoke = function invoke(obj, func, ...args) {
> 
> Do we really need invoke AND execute? Can't we just choose one?

execute (now promisify) won't be exported, it's just what call and invoke use (DRY).


> ::: toolkit/devtools/tests/unit/head_devtools.js
> @@ +4,5 @@
> >  const Cu = Components.utils;
> >  const Cr = Components.results;
> >  
> >  Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
> > +Cu.import("resource://gre/modules/devtools/AsyncUtils.jsm");
> 
> No more jsm! :(
> 
> Let's use the loader.

Done and done.


> ::: toolkit/devtools/tests/unit/test_async.js
> @@ +1,5 @@
> > +/* -*- Mode: js; js-indent-level: 2; -*- */
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +// Test AsyncUtils.safeErrorString
> 
> orly?

notrly =(

Comment 4

[Brandon Benvie [:benvie]]

Attachment: async-utils.patch

Updated patch that has the changes from the previous comment.

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

Comment 5

[Brandon Benvie [:benvie]]

Attachment: async-utils.patch

Sorry, couple comment/whitespace fixes.

Comment 6

[Brandon Benvie [:benvie]]

Paolos, what do you think about having something like these utilities in Task.jsm or some other related toolkit module?

Comment 7

[:Paolo Amadini]

(In reply to Brandon Benvie [:benvie] from comment #6)
> Paolos, what do you think about having something like these utilities in
> Task.jsm or some other related toolkit module?

The general approach I'd follow for a Toolkit module would be to start from a compelling use case and see whether it is common enough to need some helper functions. My take on most of the functions defined here is that in most cases they'll not result in something that is significantly easier to read than the syntax that these functions would replace (though the code line may be shorter).

I'd also encourage the pattern where the relevant callback-based APIs are wrapped in their own module, for example a Promise-based "Logins.jsm" could wrap the "Services.logins" API. Having slightly longer methods in these wrapper modules is probably not a big deal. On the contrary, the functions defined in the patch might induce the callers to reference the original APIs and "promisify" methods individually when calling them.

That said, if a use case that is common enough emerges (like it happened for Task.method), we can definitely work on it.

Comment 8

[Brandon Benvie [:benvie]]

(In reply to :Paolo Amadini from comment #7)
> (In reply to Brandon Benvie [:benvie] from comment #6)
> > Paolos, what do you think about having something like these utilities in
> > Task.jsm or some other related toolkit module?
> 
> The general approach I'd follow for a Toolkit module would be to start from
> a compelling use case and see whether it is common enough to need some
> helper functions. My take on most of the functions defined here is that in
> most cases they'll not result in something that is significantly easier to
> read than the syntax that these functions would replace (though the code
> line may be shorter).

I gave a presentation at the devtools workweek [1] that got a very positive response, as it can help clean up a lot of devtools code (which is why this started out as a utility for the devtools specifically). The biggest win comes from the "async" wrapper, but the others are helpful for converting existing code.

[1] https://gist.github.com/Benvie/5a1bc12e05b91ee57c7c


> I'd also encourage the pattern where the relevant callback-based APIs are
> wrapped in their own module, for example a Promise-based "Logins.jsm" could
> wrap the "Services.logins" API. Having slightly longer methods in these
> wrapper modules is probably not a big deal. On the contrary, the functions
> defined in the patch might induce the callers to reference the original APIs
> and "promisify" methods individually when calling them.

The issues we're having is mixing older APIs that use callbacks with newer APIs that use promises. Whenever we have to deal with callback APIs from Tasks we lose much of the benefit of restoring "normal" JS control flow. Ideally all the APIs we use would have a promisified wrapper API, but it's often not worth doing it for a one off usage, or where the conversion/wrapping work just hasn't been done yet. Usage of the promisify functions is definitely ugly and in an ideal world wouldn't be necessary, but for us they are a necessary evil (either that or just continue using the callback API).


> That said, if a use case that is common enough emerges (like it happened for
> Task.method), we can definitely work on it.

Where is Task.method? This sounds like what this patch's "async" function does.

Comment 9

[Brandon Benvie [:benvie]]

Comment on attachment 8378394 [details] [diff] [review]
async-utils.patch

Verbalish r+ from robcee.

Comment 10

[Brandon Benvie [:benvie]]

Attachment: async-utils.patch

Fixes loader paths test failure.

Comment 11

[Brandon Benvie [:benvie]]

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

Comment 12

[Brandon Benvie [:benvie]]

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

Comment 13

[Manish Goregaokar [:manishearth]]

You might want to add the test to the current patch on the bug, currently the test is missing.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a30d2ed1597
https://hg.mozilla.org/mozilla-central/rev/671b3044b166

Comment 15

[Brandon Benvie [:benvie]]

(In reply to Manish Goregaokar [:manishearth] from comment #13)
> You might want to add the test to the current patch on the bug, currently
> the test is missing.

Yeah, I changed the name of it and forgot to addremove. I landed a followup with the test almost immediately though (the second changeset).

Comment 16

[Manish Goregaokar [:manishearth]]

(In reply to Brandon Benvie [:benvie] from comment #15)
> (In reply to Manish Goregaokar [:manishearth] from comment #13)
> > You might want to add the test to the current patch on the bug, currently
> > the test is missing.
> 
> Yeah, I changed the name of it and forgot to addremove. I landed a followup
> with the test almost immediately though (the second changeset).

I know, I'm saying you should add it as an attachment to the bug itself, for future reference :)

Comment 17

[Brandon Benvie [:benvie]]

Attachment: async-utils-test.patch

(In reply to Manish Goregaokar [:manishearth] from comment #16)
> I know, I'm saying you should add it as an attachment to the bug itself, for
> future reference :)

Ah, you're right!

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Any reason you opted for two separate functions promiseInvoke, promiseCall? I think this could be done with one method, folding promisify since it will then only be used once. Call examples:


// With a this object
AsyncUtils.promisify(this.mymethod.bind(this), "arg1", 2, "three");

// Without a this object:
AsyncUtils.promisify(myothermethod, "arg1", 2, "three");


I guess thats a little longer than using promiseInvoke(this, this.mymethod, "arg1", 2, "three"), but not by much.

Comment 19

[Brandon Benvie [:benvie]]

(In reply to Philipp Kewisch [:Fallen] from comment #18)
> Any reason you opted for two separate functions promiseInvoke, promiseCall?
> I think this could be done with one method, folding promisify since it will
> then only be used once. Call examples:
> 
> 
> // With a this object
> AsyncUtils.promisify(this.mymethod.bind(this), "arg1", 2, "three");
> 
> // Without a this object:
> AsyncUtils.promisify(myothermethod, "arg1", 2, "three");
> 
> 
> I guess thats a little longer than using promiseInvoke(this, this.mymethod,
> "arg1", 2, "three"), but not by much.

In our code, invoking a method would be the more common usage so that would be the one I'd want to keep.  A better argument could be made for removing promiseCall, since you can replicate it using `promiseInvoke(null, fn, ...)`. I could be convinced that promiseCall should be removed, I was just trying to cover the two types of usage without requiring incantations (like `obj.method.bind(obj)`) or extraneous parameters.

This is kind of analogous to me to `Promise.prototype.catch`, which is just `this.then(undefined, onReject)`; a thin wrapper around something that is common enough to be useful to have the shortcut.

Comment 20

[:Paolo Amadini]

(In reply to Brandon Benvie [:benvie] from comment #8)
> Where is Task.method? This sounds like what this patch's "async" function
> does.

It is being worked on in bug 966182, and it is indeed the equivalent of "async" here. For some reason I thought it had landed already.