966182 - Implement Task.async

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: implements Task.Method

Consumers of Task often declare methods that spawn and return tasks that are bound to the method's 'this' object and access its arguments, like this simple object with a 'greet' method that has a 'name' parameter and spawns a task to send a greeting and return its reply:

let greeter = {
  message: "Hello, NAME!",
  greet: function(name) {
    return Task.spawn((function* () {
      return yield sendGreeting(this.message.replace(/NAME/, name));
    }).bind(this);
  })
};

Here are some examples in the codebase:

https://github.com/mozilla/gecko-dev/blob/a8d1e0fc125318b7060f7743a844c7b2c3870c28/toolkit/modules/Log.jsm#L598-L611
https://github.com/mozilla/gecko-dev/blob/a8d1e0fc125318b7060f7743a844c7b2c3870c28/dom/apps/src/Webapps.jsm#L577-L616
https://github.com/mozilla/gecko-dev/blob/a8d1e0fc125318b7060f7743a844c7b2c3870c28/services/healthreport/healthreporter.jsm#L1324-L1366

These declarations are verbose, complex, add an extraneous level of indentation, and expose footguns; so it'd be useful to simplify them with some sugar that sets the "this" object of such a "task method" and exposes the method arguments to the task.

Here's a patch that provides that sugar via a Task.Method function.  It enables you to rewrite the sample code above succinctly:

let greeter = {
  message: "Hello, NAME!",
  greet: Task.Method(function* (name) {
    return yield sendGreeting(this.message.replace(/NAME/, name));
  })
};

While maintaining identical semantics:

greeter.greet("Mitchell").then((reply) => { … }); // behaves the same

The patch itself is relatively simple, and it comes with tests.  The hardest part of creating it was picking the name!

I considered Task.bind, Task.prep, and Task.wrap, among others, and eventually chose Task.Method because it returns a function that behaves like a method when called like one.  Nevertheless, I'm agnostic about the name and happy to change it, so wordsmith away!

One side-effect of calling something "Method" with a capital-M is that it looks like a constructor.  In this case, it's a pseudo-constructor, because it doesn't need a 'new' keyword, but it tolerates one if present.

I briefly considered making it a real constructor but opted to optimize for brevity.  I also considered making Task itself the function that returns the method, but that would make it incompatible with task.js.

Here's a try run:

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

Comment 1

[:Paolo Amadini]

Comment on attachment 8368432 [details] [diff] [review]
patch v1: implements Task.Method

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

This is a very good patch and an improvement that will be generally useful to Task users, thanks!

(In reply to Myk Melez [:myk] [@mykmelez] from comment #0)
> The hardest part of creating it was picking the name!

We came to the same conclusion independently, so "Task.method" it is!

> One side-effect of calling something "Method" with a capital-M is that it
> looks like a constructor.  In this case, it's a pseudo-constructor, because
> it doesn't need a 'new' keyword, but it tolerates one if present.

I'd use lowercase if you can't do "object.taskMethod instanceof Task.Method". Also, we should recommend one style of usage, I'd go without the "new" keyword. I'd not include the "new" case in tests, to make that clear in case tests are taken as a usage example.

Also, I'd implement Task.method in terms of Task.spawn and Function.bind. Task.method should throw if its argument is not a function (and there should be tests for that).

We should have a success test for the case where Task.method is called with a function that is not a generator. This can happen when someone writes an old-style generator and at some point the last "yield" keyword is removed.

Comment 2

[:Paolo Amadini]

Myk, are you still working on this? In the meantime devtools has implemented its own helper in bug 974065, confirming that this is indeed useful work.

Comment 3

[Brandon Benvie [:benvie]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #0)
> The hardest part of creating it was picking the name!
> 
> I considered Task.bind, Task.prep, and Task.wrap, among others, and
> eventually chose Task.Method because it returns a function that behaves like
> a method when called like one.  Nevertheless, I'm agnostic about the name
> and happy to change it, so wordsmith away!

The reason I ended up naming it "async" is because the behavior here is what (likely) ES7 async function's will be called. This is modeled after, amongst other languages, C#'s async/await [1].

http://msdn.microsoft.com/en-us/library/hh191443.aspx

Comment 4

[Brandon Benvie [:benvie]]

(also http://wiki.ecmascript.org/doku.php?id=strawman:async_functions and https://github.com/lukehoban/ecmascript-asyncawait)

Comment 5

[Myk Melez [:myk] [@mykmelez]]

I'm still working on it!  Sorry for the delay responding to your helpful review comments!  I'm back from vacation today and will post an updated patch shortly after catching up on the goings-on while I was out.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses feedback

(In reply to :Paolo Amadini from comment #1)
> I'd use lowercase if you can't do "object.taskMethod instanceof
> Task.Method". Also, we should recommend one style of usage, I'd go without
> the "new" keyword. I'd not include the "new" case in tests, to make that
> clear in case tests are taken as a usage example.

Yup, that makes sense.  I used lowercase and removed the "new" case from tests.

https://github.com/mykmelez/gecko-dev/commit/6d7feaae00aeaaa44b62c54a0394422cb81f0242


> Also, I'd implement Task.method in terms of Task.spawn and Function.bind.

Function.bind would require binding the method to the "this" object when Task.method is invoked; but methods defined as part of an object literal declaration won't have access to the "this" object yet:

  let object = {
    foo: Task.method(function* () { … }) // We don't have "this" yet.
  }

And even for methods defined after-the-fact, using Function.bind would require passing the "this" object to Task.method, making this sugar a little less sweet:

  let object = {};
  object.foo = Task.method(function* () { … }, /* "this" object */ object);


And as for implementing Task.method in terms of Task.spawn, it seems better to implement both in terms of a third function that factors out their common functionality, such that neither has to sprout an additional parameter to conditionally satisfy the other's use case, which might confuse consumers.

If Task.spawn also bound its task, however, then it would be possible to implement one in terms of the other, but it would be Task.spawn that would be implemented in terms of Task.method, since in that case Task.spawn would just be the equivalent of calling Task.method and then immediately calling the result.

(And, in any case, if we decide to do that, then we should do it in a different bug, since it's a separable concern.)


> Task.method should throw if its argument is not a function (and there should
> be tests for that).

Good point, fixed.

https://github.com/mykmelez/gecko-dev/commit/d5582e7799872b310eb95e1e2f7a3df1ddf34188


> We should have a success test for the case where Task.method is called with
> a function that is not a generator. This can happen when someone writes an
> old-style generator and at some point the last "yield" keyword is removed.

Indeed, test added (fortunately, it just worked!).

https://github.com/mykmelez/gecko-dev/commit/803249fcffad2e78ee441888320c8f9b0753c32e


(In reply to Brandon Benvie [:benvie] from comment #3)
> The reason I ended up naming it "async" is because the behavior here is what
> (likely) ES7 async function's will be called. This is modeled after, amongst
> other languages, C#'s async/await [1].
> 
> http://msdn.microsoft.com/en-us/library/hh191443.aspx

(In reply to Brandon Benvie [:benvie] from comment #4)
> (also http://wiki.ecmascript.org/doku.php?id=strawman:async_functions and
> https://github.com/lukehoban/ecmascript-asyncawait)

Indeed!  This wasn't clear to me at first, since the proposal doesn't mention how the function will be bound; but ecmascript-asyncawait uses q.async, which binds its generator argument to the "this" object:

  https://github.com/kriskowal/q/blob/v1/q.js#L1209

So I changed the name of the function to "async" and updated its documentation accordingly.

https://github.com/mykmelez/gecko-dev/commit/d385b862c249fd9fccfada2d81a2f643e616a9da

(Paolo: I can change it back if you disagree; it's simply a matter of reverting that commit on the branch I'm using to develop this patch; but I think benvie has a point about the value of consistency with the ES7 strawman proposal, even if there's no guarantee that the proposal will be adopted.  Pave the cowpaths!)

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: fixes test, adds another test

This update fixes a bug that caused one of the test functions to be skipped.

https://github.com/mykmelez/gecko-dev/commit/623c2fe0e884fa640d9b1a1311c849480f900c84


It also adds a test that shows why the "async" implementation in bug 974065, which is implemented in terms of Task.spawn, is insufficient: if the task returns a function, that implementation would call it and resolve to its return value; whereas it should resolve to the returned function itself:

  // This should yield the inner function, but instead it yields the inner's
  // return value ("foo"), given the "async" implementation in bug 974065.
  require(…).async(function () { return function () { return "foo" } })();

  // This correctly yields the inner function, given the implementation
  // in this bug.
  yield Task.async(function () { return function () { return "foo" } })();

https://github.com/mykmelez/gecko-dev/commit/c68f8256cbe9641da90bb5effebbdbc638fd9b88

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #7)
>   // This should yield the inner function, but instead it yields the inner's
>   // return value ("foo"), given the "async" implementation in bug 974065.
>   require(…).async(function () { return function () { return "foo" } })();

Erm, here I meant to write:

  yield require(…).async(function () { return function () { return "foo" } })();


Here's a complete test you can paste into the Browser Console to verify this for yourself:

  let {require} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
  let {async} = require("devtools/async-utils");
  async(function () { return function () { return "foo" } })().then(console.log);
  // Logs "foo".

Versus (with the patch in this bug applied):

  let {Task} = Cu.import("resource://gre/modules/Task.jsm", {});
  Task.async(function () { return function () { return "foo" } })().then(console.log);
  // Logs function ().

Comment 9

[Brandon Benvie [:benvie]]

Comment on attachment 8380140 [details] [diff] [review]
patch v3: fixes test, adds another test

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

Looking good. You should add a test that checks that throwing from the async function results in a rejected promise.

::: toolkit/modules/tests/xpcshell/test_task.js
@@ +279,5 @@
> +        return [param, this.prop];
> +      })
> +    };
> +
> +    (

No need for parens here.

Comment 10

[:Paolo Amadini]

Comment on attachment 8380140 [details] [diff] [review]
patch v3: fixes test, adds another test

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

I thought about whether we should call this Task.async or Task.method, and we should probably go for the former given that it is already used elsewhere in a consistent manner.

My only concern is that "async" doesn't convey any meaning to people that are not already familiar with async functions.

While the syntax "async function(args) {}" makes it clear that we are handling a function, "Task.async(function* (args) {})" does not - it might seem that Task.async returns a promise like Task.spawn does.

In particular, of these sub-task invocations:
  yield Task.spawn(function * () { ... })
  yield Task.async(function * () { ... })

The first is correct and the second is wrong. Of these declarations:

  MyObject.prototype.asyncFn = Task.spawn(function * () { ... })
  MyObject.prototype.asyncFn = Task.async(function * () { ... })

The first is wrong and the second is correct.

My take is that we can use these "unclear" names, but we should provide developers with a clear way to detect any usage mistake.

To do this, we can taint the returned function with an isAsyncFunction property set to true. Whenever an async function is directly passed to "Task.spawn", "yield", "Promise.resolve", or as the return value of a "then" callback, we raise an informative exception (or equivalent rejection). The message may vary depending on context, but something along the line of the following message will do:

"Cannot use an async function in place of a promise. You should either invoke the async function first, or use 'Task.spawn' instead of 'Task.async'."

This might prevent some kind of indirection for which async functions are passed around intentionally, but I think this is fine. Generally, resolution values are objects with async functions, not the functions themselves. We don't have strong typing like C#, but we can use these strategies to facilitate developers and avoid pitfalls.

Myk, can you please add these checks to the current patch? (I guess that a check in Task.spawn and one in Promise.resolve would give us full coverage.)

::: toolkit/modules/Task.jsm
@@ +169,5 @@
> +   * };
> +   *
> +   * While maintaining identical semantics:
> +   *
> +   * greeter.greet("Mitchell").then((reply) => { … }); // behaves the same

Replace non-ASCII character.

@@ +196,5 @@
>      this.value = aValue;
>    }
>  };
>  
> +function createTask(aTask, aApply) {

createAsyncFunction?

@@ +202,5 @@
> +    let result = aTask;
> +    if (aTask && typeof(aTask) == "function") {
> +      try {
> +        // Let's call into the function ourselves.
> +        result = aApply ? aTask.apply(this, arguments) : aTask();

The aApply argument still sounds weird to me. Can we always call apply here, and implement "spawn" as "return createAsyncFunction(aTask).apply(undefined);"? It also makes more evident that we are invoking the returned function.

::: toolkit/modules/tests/xpcshell/test_task.js
@@ +243,5 @@
>    });
>  });
> +
> +add_test(function test_async()
> +{

Please split this function in several separate add_test calls.

@@ +250,5 @@
> +    do_check_eq(3, prop);
> +  }
> +
> +  Task.spawn(function* () {
> +    // Ensure async functions work when yielded within a generator.

This test as stated is confusing, because we're not yielding the async function but the promise it returns. Yielding a promise is already tested elsewhere.

I think we should only have one test that checks that the function returns a promise - whether we implement it with a task or with "then" calls is indifferent, but we don't need both.

@@ +256,5 @@
> +    let object = {
> +      prop: 3,
> +
> +      asyncFunction: Task.async(function* (param) {
> +        return [param, this.prop];

Maybe we can just put a do_check_eq(this, object) here and return the parameter, making the "check" function unnecessary.

@@ +264,5 @@
> +    // Ensure we can call the async function.
> +    check(yield object.asyncFunction(6));
> +
> +    // Ensure we can call the same async function twice.
> +    check(yield object.asyncFunction(6));

We should call the function with a different argument, to ensure that it doesn't return the same promise.

@@ +284,5 @@
> +      // Ensure we can call the async function.
> +      object.asyncFunction(6)
> +    ).then(
> +      check
> +    ).then(function () {

global nit: I'd inline "check" with "then" since it's not a statement, while other calls at the same indentatoin level are. In any case, we're probably replacing this code with something else in the end.

@@ +314,5 @@
> +      check(yield object.asyncFunction(6));
> +    });
> +  }).then(function () {
> +    // Ensure an async function that returns a function resolves to the function
> +    // itself instead of calling the function and resolving to its return value.

It seems that this is a corner case, and the bug it surfaced only applies to the less common "Task.async(function () {" as opposed to "Task.async(function* () {", since the latter would behave like a generator in any case.

In any case, I think we can leave this correctness test, since we have it now.

Comment 11

[Florian Bender]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #0)
> let greeter = {
>   message: "Hello, NAME!",
>   greet: function(name) {
>     return Task.spawn((function* () {
>       return yield sendGreeting(this.message.replace(/NAME/, name));
>     }).bind(this);
>   })
> };

Couldn't this just be rewritten to: 

let greeter = {
  message: "Hello, NAME!",
  greet: (name) => Task.spawn(function* () {
    return yield sendGreeting(this.message.replace(/NAME/, name));
  })
};

… thus not needing any (more) sugar? Or did I get something wrong here?

Comment 12

[Myk Melez [:myk] [@mykmelez]]

(In reply to Florian Bender from comment #11)
> Couldn't this just be rewritten to: 
> 
> let greeter = {
>   message: "Hello, NAME!",
>   greet: (name) => Task.spawn(function* () {
>     return yield sendGreeting(this.message.replace(/NAME/, name));
>   })
> };
> 
> … thus not needing any (more) sugar? Or did I get something wrong here?

If this worked, then it would be sweet enough, even though it contains an extra function declaration.  But the function* isn't bound to greeter, so this.message isn't greeter.message inside it.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #12)
> If this worked, then it would be sweet enough, even though it contains an
> extra function declaration.  But the function* isn't bound to greeter, so
> this.message isn't greeter.message inside it.

And you can't simply bind it to the *this* object, because the fat arrow function isn't bound to greeter either.  You have to either bind it to *greeter* explicitly:

let greeter = {
  message: "Hello, NAME!",
  greet: (name) => Task.spawn(function* () {
    return yield sendGreeting(this.message.replace(/NAME/, name));
  }.bind(greeter))
};

Or wrap it in a regular function and then bind it to *this*:

let greeter = {
  message: "Hello, NAME!",
  greet: function (name) {
    return Task.spawn(function* () {
      return yield sendGreeting(this.message.replace(/NAME/, name));
    }.bind(this));
  }
};

Neither of which is very sweet.  Hence this bug!

Comment 14

[Florian Bender]

Uh, missed that. But I thought the whole point of fat arrow functions were that they don't create a new context? So this should work: 

let greeter = {
  message: "Hello, NAME!",
  greet: (name) => Task.spawn(function* () {
    return yield sendGreeting(this.message.replace(/NAME/, name));
  }.bind(this))
};

I agree it's not über-nice.

There is no fat-arrow-generator, is there?

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to Florian Bender from comment #14)
> Uh, missed that. But I thought the whole point of fat arrow functions were
> that they don't create a new context? So this should work: 
> 
> let greeter = {
>   message: "Hello, NAME!",
>   greet: (name) => Task.spawn(function* () {
>     return yield sendGreeting(this.message.replace(/NAME/, name));
>   }.bind(this))
> };

It's true that arrow functions don't create a new context, but that's exactly why the code snippet doesn't work: the "this" object in the arrow function is the "this" object in the context in which it's *declared*, not the context in which it's *called*.  So "this" is not "greeter" inside the arrow function, even if the arrow function is called as a method of greeter via the "greeter.greet" syntax.

Here's a simple demonstration of this behavior that you can run in the Browser Console:

  // Logs "Boo!", not "Yay!".
  let message = "Boo!"; let greeter = { message: "Yay!", greet: () => this.message }; greeter.greet();


> There is no fat-arrow-generator, is there?

I don't think so, but that probably still wouldn't address the use case, since it would probably still assign "this" as per other arrow functions.

Ultimately we need an "async function" syntax to make this really sweet, whether that's literally "async function", "function^", "function!", or something else entirely (cf. <http://wiki.ecmascript.org/doku.php?id=strawman:async_functions>).

In the meantime, we can get pretty close with the functionality proposed in this bug.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: addresses review remarks

(In reply to :Paolo Amadini from comment #10)

> I thought about whether we should call this Task.async or Task.method, and
> we should probably go for the former given that it is already used elsewhere
> in a consistent manner.
> 
> My only concern is that "async" doesn't convey any meaning to people that
> are not already familiar with async functions.
> 
> While the syntax "async function(args) {}" makes it clear that we are
> handling a function, "Task.async(function* (args) {})" does not - it might
> seem that Task.async returns a promise like Task.spawn does.

The other issue is that "async" is an adjective, so while it may be ideal for the decorator in C# and the proposed ES7 syntax, it's suboptimal as the name of a function.

Nevertheless, "method" is also suboptimal, since it's a noun.  And I couldn't find a simple verb that conveyed the function of the function clearly enough (even after trawling aquatic biology texts to find words describing activities in which fish engage before they spawn!).

Which means that whatever we use will need to be learned and remembered by its users as a distinctive and evocative idiom (per the principles of idiomatic interface design).  For which "async" seems as good as any other name, if not better.


> In particular, of these sub-task invocations:
>   yield Task.spawn(function * () { ... })
>   yield Task.async(function * () { ... })
> 
> The first is correct and the second is wrong. Of these declarations:
> 
>   MyObject.prototype.asyncFn = Task.spawn(function * () { ... })
>   MyObject.prototype.asyncFn = Task.async(function * () { ... })
> 
> The first is wrong and the second is correct.
> 
> My take is that we can use these "unclear" names, but we should provide
> developers with a clear way to detect any usage mistake.

I see what you mean.


> To do this, we can taint the returned function with an isAsyncFunction
> property set to true. Whenever an async function is directly passed to
> "Task.spawn", "yield", "Promise.resolve", or as the return value of a "then"
> callback, we raise an informative exception (or equivalent rejection). The
> message may vary depending on context, but something along the line of the
> following message will do:
> 
> "Cannot use an async function in place of a promise. You should either
> invoke the async function first, or use 'Task.spawn' instead of
> 'Task.async'."
> 
> This might prevent some kind of indirection for which async functions are
> passed around intentionally, but I think this is fine. Generally, resolution
> values are objects with async functions, not the functions themselves. We
> don't have strong typing like C#, but we can use these strategies to
> facilitate developers and avoid pitfalls.

I'm less sanguine about imposing this limitation, since scripting language developers seem fond of indirection and meta-programming.  But it's trivial to allow async function passing in the future, if folks come up with use cases; whereas it's hard to put the cat back in the bag if we enable it today and find that folks keep tripping over it.  So this seems wise.


> Myk, can you please add these checks to the current patch? (I guess that a
> check in Task.spawn and one in Promise.resolve would give us full coverage.)

Done!

[task-sugar a82a752] prevent accidental usage of async function in place of promise


> ::: toolkit/modules/Task.jsm
> @@ +169,5 @@
> > +   * };
> > +   *
> > +   * While maintaining identical semantics:
> > +   *
> > +   * greeter.greet("Mitchell").then((reply) => { … }); // behaves the same
> 
> Replace non-ASCII character.

Ok.

[task-sugar 30096e7] replace non-ASCII character with ASCII simulacrum


> @@ +196,5 @@
> >      this.value = aValue;
> >    }
> >  };
> >  
> > +function createTask(aTask, aApply) {
> 
> createAsyncFunction?

Yep.

[task-sugar 07b2d70] rename createTask to createAsyncFunction


> @@ +202,5 @@
> > +    let result = aTask;
> > +    if (aTask && typeof(aTask) == "function") {
> > +      try {
> > +        // Let's call into the function ourselves.
> > +        result = aApply ? aTask.apply(this, arguments) : aTask();
> 
> The aApply argument still sounds weird to me. Can we always call apply here,
> and implement "spawn" as "return
> createAsyncFunction(aTask).apply(undefined);"? It also makes more evident
> that we are invoking the returned function.

Yes, we can do that, although we might as well use "call" in this case, since we don't need to pass arguments.  Also, note that we don't *have* to use call/apply at all, although it does make the returned function call more evident.

Finally, note that binding "this" to the undefined value behaves differently when the bound function is declared in a strict mode context.  Per <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call#Parameters>:

    The value of this provided for the call to fun. Note that this may not be the actual value seen by
    the method: if the method is a function in non-strict mode code, null and undefined will be replaced
    with the global object, and primitive values will be boxed.

So I added tests for both strict and non-strict modes.

[task-sugar 0024441] get rid of aApply parameter


> ::: toolkit/modules/tests/xpcshell/test_task.js
> @@ +243,5 @@
> >    });
> >  });
> > +
> > +add_test(function test_async()
> > +{
> 
> Please split this function in several separate add_test calls.

Great idea, the function was getting unwieldy.

[task-sugar ccad8d2] split async function tests into multiple test functions


> @@ +250,5 @@
> > +    do_check_eq(3, prop);
> > +  }
> > +
> > +  Task.spawn(function* () {
> > +    // Ensure async functions work when yielded within a generator.
> 
> This test as stated is confusing, because we're not yielding the async
> function but the promise it returns. Yielding a promise is already tested
> elsewhere.
> 
> I think we should only have one test that checks that the function returns a
> promise - whether we implement it with a task or with "then" calls is
> indifferent, but we don't need both.

Ok, I removed the "then"-based test, leaving the task-based one.

[task-sugar ac4a981] remove duplicative then-based test that async function returns promise


> @@ +256,5 @@
> > +    let object = {
> > +      prop: 3,
> > +
> > +      asyncFunction: Task.async(function* (param) {
> > +        return [param, this.prop];
> 
> Maybe we can just put a do_check_eq(this, object) here and return the
> parameter, making the "check" function unnecessary.

Good idea.  I'm not a fan of refactored test assertions anyway, as they make it harder to read the test code.

[task-sugar dadd8e3] unrefactor async function test assertions


> @@ +264,5 @@
> > +    // Ensure we can call the async function.
> > +    check(yield object.asyncFunction(6));
> > +
> > +    // Ensure we can call the same async function twice.
> > +    check(yield object.asyncFunction(6));
> 
> We should call the function with a different argument, to ensure that it
> doesn't return the same promise.

Yep.

[task-sugar 06d6684] ensure multiple calls don't return the same promise


> @@ +284,5 @@
> > +      // Ensure we can call the async function.
> > +      object.asyncFunction(6)
> > +    ).then(
> > +      check
> > +    ).then(function () {
> 
> global nit: I'd inline "check" with "then" since it's not a statement, while
> other calls at the same indentatoin level are. In any case, we're probably
> replacing this code with something else in the end.

Indeed, this is all gone now.


> @@ +314,5 @@
> > +      check(yield object.asyncFunction(6));
> > +    });
> > +  }).then(function () {
> > +    // Ensure an async function that returns a function resolves to the function
> > +    // itself instead of calling the function and resolving to its return value.
> 
> It seems that this is a corner case, and the bug it surfaced only applies to
> the less common "Task.async(function () {" as opposed to
> "Task.async(function* () {", since the latter would behave like a generator
> in any case.
> 
> In any case, I think we can leave this correctness test, since we have it
> now.

I originally added this simply to help figure out why my implementation couldn't be the simpler one in bug 974065.  But I can imagine someone reintroducing the issue it identifies.  So I think it's worth keeping it.


(In reply to Brandon Benvie [:benvie] from comment #9)
> Comment on attachment 8380140 [details] [diff] [review]
> patch v3: fixes test, adds another test
> 
> Review of attachment 8380140 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good. You should add a test that checks that throwing from the async
> function results in a rejected promise.

Yup.

[task-sugar 0b3dcf8] ensure async function that throws rejects its promise


> ::: toolkit/modules/tests/xpcshell/test_task.js
> @@ +279,5 @@
> > +        return [param, this.prop];
> > +      })
> > +    };
> > +
> > +    (
> 
> No need for parens here.

Indeed, I was simply trying to structure the promise chain in way that made it more readable.  But other changes have gotten rid of that chain in any case.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Erm, I forgot to linkify the commit references before attaching the patch with the comment!  Here they are (in case it helps with the review).

> [task-sugar a82a752] prevent accidental usage of async function in place of
> promise

https://github.com/mykmelez/gecko-dev/commit/a82a752


> [task-sugar 30096e7] replace non-ASCII character with ASCII simulacrum

https://github.com/mykmelez/gecko-dev/commit/30096e7


> [task-sugar 07b2d70] rename createTask to createAsyncFunction

https://github.com/mykmelez/gecko-dev/commit/07b2d70


> [task-sugar 0024441] get rid of aApply parameter

https://github.com/mykmelez/gecko-dev/commit/0024441


> [task-sugar ccad8d2] split async function tests into multiple test functions

https://github.com/mykmelez/gecko-dev/commit/ccad8d2


> [task-sugar ac4a981] remove duplicative then-based test that async function
> returns promise

https://github.com/mykmelez/gecko-dev/commit/ac4a981


> [task-sugar dadd8e3] unrefactor async function test assertions

https://github.com/mykmelez/gecko-dev/commit/dadd8e3


> [task-sugar 06d6684] ensure multiple calls don't return the same promise

https://github.com/mykmelez/gecko-dev/commit/06d6684


> [task-sugar 0b3dcf8] ensure async function that throws rejects its promise

https://github.com/mykmelez/gecko-dev/commit/0b3dcf8

Comment 18

[Florian Bender]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #15)

Alright, I surrender. Now I got it, thanks!

Comment 19

[:Paolo Amadini]

Comment on attachment 8384352 [details] [diff] [review]
patch v4: addresses review remarks

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

Great work.

::: toolkit/modules/Promise.jsm
@@ +445,5 @@
> +  if (aValue && typeof(aValue) == "function" && aValue.isAsyncFunction) {
> +      throw "Cannot resolve a promise with an async function. " +
> +            "You should either invoke the async function first " +
> +            "or use 'Task.spawn' instead of 'Task.async' to start " +
> +            "the Task and return its promise.";

I think that "throw new Error(...)" is better as it allows capturing a stack trace. Is it correct?

::: toolkit/modules/tests/xpcshell/test_Promise.js
@@ +534,5 @@
> +    try {
> +      Promise.resolve(Task.async(function* () {}));
> +      throw "unexpected success calling Promise.resolve with async function argument";
> +    } catch(ex) {
> +      do_check_eq(ex, "Cannot resolve a promise with an async function. " +

"Assert.throws" can now be used in xpcshell tests to simplify this. It also accepts regular expressions to match parts of the message. From memory:

Assert.throws(() => Promise.resolve(Task.async(function* () {})),
              /Cannot resolve a promise with an async function/);

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: fixes nits in r+ed patch

(In reply to :Paolo Amadini from comment #19)
> ::: toolkit/modules/Promise.jsm
> @@ +445,5 @@
> > +  if (aValue && typeof(aValue) == "function" && aValue.isAsyncFunction) {
> > +      throw "Cannot resolve a promise with an async function. " +
> > +            "You should either invoke the async function first " +
> > +            "or use 'Task.spawn' instead of 'Task.async' to start " +
> > +            "the Task and return its promise.";
> 
> I think that "throw new Error(...)" is better as it allows capturing a stack
> trace. Is it correct?

Yes, that's right!

https://github.com/mykmelez/gecko-dev/commit/ec19d6fcec518027c76d87f2dcccd1c5f3666a83

(I made them TypeError objects as it seems accurate and is more precise.  I also updated the "aTask argument not a function" exception, taking advantage of the opportunity to wordsmith it a bit, turning it into the positive assertion "aTask argument must be a function".)


> ::: toolkit/modules/tests/xpcshell/test_Promise.js
> @@ +534,5 @@
> > +    try {
> > +      Promise.resolve(Task.async(function* () {}));
> > +      throw "unexpected success calling Promise.resolve with async function argument";
> > +    } catch(ex) {
> > +      do_check_eq(ex, "Cannot resolve a promise with an async function. " +
> 
> "Assert.throws" can now be used in xpcshell tests to simplify this. It also
> accepts regular expressions to match parts of the message. From memory:
> 
> Assert.throws(() => Promise.resolve(Task.async(function* () {})),
>               /Cannot resolve a promise with an async function/);

Ah, nice!

https://github.com/mykmelez/gecko-dev/commit/11235518cb69565a3641e4b4fbdd927d8cf16e97


Here's the version of the patch with those nits fixed.  This is the version I'll push.  I'll just await these try results first:

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

Comment 21

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bfcf691690a

Comment 22

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

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/19fe1e621c5c for introducing a new hazard: https://tbpl.mozilla.org/php/getParsedLog.php?id=35697398&tree=Mozilla-Inbound

Comment 23

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

Unbacked out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b8551123b3da because a retrigger on an earlier push shows this wasn't the cause of the hazard. Sorry for the churn. :(

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bfcf691690a

Comment 25

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

This should be documented on https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm

Comment 26

[Axel Grude [:realRaven]]

(In reply to Philipp Kewisch [:Fallen] from comment #25)
> This should be documented on
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> Task.jsm

+1 - could somebody please add an example for Task.async() ?

Also it would be helpful to see some way of using legacy (or avoiding) generators for older Gecko platforms such as Postbox.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Myk or Paolo, any chance either of you could document this?

Comment 28

[Axel Grude [:realRaven]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #27)
> Myk or Paolo, any chance either of you could document this?

Also can somebody document on mdn what Gecko / Thunderbird version Task.async() landed in? I would like to use this for my minver in the next addons versions. 

Task.async is essential when dealing with IMAP folders and will come in extremely handy for bulk message management and filtering tasks in the future.

Comment 29

[Myk Melez [:myk] [@mykmelez]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #27)
> Myk or Paolo, any chance either of you could document this?

Yes, I'll add documentation for Task.async to the MDN page about Task.jsm, basing it on the code comments in Task.jsm.


(In reply to Axel Grude [:realRaven] from comment #28)
> Also can somebody document on mdn what Gecko / Thunderbird version
> Task.async() landed in? I would like to use this for my minver in the next
> addons versions. 

Yes, I'll add the version it landed in (30) to the documentation on MDN.


> Task.async is essential when dealing with IMAP folders and will come in
> extremely handy for bulk message management and filtering tasks in the
> future.

I'm glad that it's useful to you!

Comment 30

[Myk Melez [:myk] [@mykmelez]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #29)
> Yes, I'll add documentation for Task.async to the MDN page about Task.jsm,
> basing it on the code comments in Task.jsm.
…
> Yes, I'll add the version it landed in (30) to the documentation on MDN.

I did both in revision 1017582:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm$revision/1017582