Aborting tasks with async hops

[shaseley]

FF folks recently added test cases (thanks!) for aborting ongoing tasks from within the task itself. The second test fails in Chromium, which is essentially boils down to:

  const controller = new TaskController();
  const signal = controller.signal;
  const p = scheduler.postTask(async () => {
    await new Promise(resolve => setTimeout(resolve, 0));
    // Should |p| be resolved or rejected?
    controller.abort();
  });

In Chromium, p is not rejected because we resolve the postTask promise with the promise returned by the async function, so when the controller aborts it's a no-op. I believe this matches the current spec text (5.1 of the schedule a task to invoke a callback algorithm) since there we resolve the promise with the result of invoking a callback, which should return a promise in the async function case.

But, I think the behavior of the test makes more sense and that we should update the spec to match. I think that would involve "awaiting" the result of invoking the callback and resolving the promise result with that value (or propagating the error). @sefeng211 is that what you did in the FF implementation?

[smaug----]

Isn't this about Promise chaining. Run the following in a browser's web console and check how the outer Promise is fullfilled:

var p = new Promise((resolve) => { resolve(new Promise((resolveInner) => { setTimeout(resolveInner, 1000); })); }); console.log(p); setTimeout(() => {console.log(p)}, 2000 );

But I could very well miss still something.

[smaug----]

A bit modified version which rejects the inner but resolves the outer
var p = new Promise((resolve) => { resolve(new Promise((resolveInner, rejectInner) => { setTimeout(rejectInner, 1000); })); }); console.log(p); setTimeout(() => {console.log(p)}, 2000 );

that is close to the testcase and p is rejected because of 6.2

[shaseley]

I was thinking this should be covered by promise chaining at first too, but I think it's different -- in the test case we're rejecting the outer after it was resolved with the inner promise, something like:

var p = new Promise((resolve, rejectOuter) => { resolve(new Promise((resolveInner, rejectInner) => { setTimeout(rejectOuter, 1000); })); }); console.log(p); setTimeout(() => {console.log(p)}, 2000 );

(controller.abort == rejectOuter).

^^ that gets stuck at for me in FF and Chrome, which makes sense because the outer is dependent on inner. I believe this would be the scenario if the abort steps in 6.2 runs and result is already resolved with a pending promise.

[smaug----]

ah, I see. But the promise isn't yet resolved (because of the inner), yet does Chrome in the test get resolved p... because the function eventually returns. (I'll need to play with this a bit more tomorrow :) )

[smaug----]

I filed a Gecko bug about this https://bugzilla.mozilla.org/show_bug.cgi?id=1767087
It is possible that an optimization in Gecko or Spidermonkey kicks in when it shouldn't, @arai-a might know better.

[arai-a]

This was mis-optimization in SpiderMonkey, and p shouldn't be rejected.

Resolving promise should set [[AlreadyResolved]].[[Value]] to true, to prevent subsequent resolve/reject call.
https://tc39.es/ecma262/#sec-promise-resolve-functions

Then, SpiderMonkey doesn't create Promise Resolve Function and Promise Reject Function if they're known not to be exposed to user JS code, but resolve/reject is done only from C++ code internally or with public APIs,
and in that case the [[AlreadyResolved]].[[Value]] check wasn't performed.

Fixing in bug 1767087.

[arai-a]

Fixed by web-platform-tests/wpt@146851f

[shaseley]

Thanks @arai-a and @smaug----!

Thinking about the behavior a bit more, the current behavior here seems reasonable? To abort a running async task (e.g. in response to user input), the task's promise would need to be rejected along with the rest of the task. Using signal.throwIfAborted() in userland at various checkpoints accomplishes this. It might be desirable to do this automatically on signal abort, but I think we'd need signal propagation of some kind — including through the JS layer. We might explore that at some point in conjunction with async task priority propagation.

[smaug----]

At least we could close this issue, since this became a debugging session :)