Ecma262: Missing abrupt completion handling in AsyncFromSyncIteratorContinuation?

[anba]

AsyncFromSyncIteratorContinuation can return abruptly in step 5

1. Let valueWrapper be ? PromiseResolve(%Promise%, « value »).

But all callers assume it always returns normally.

Do the callers need to be updated to use ? AsyncFromSyncIteratorContinuation or is it actually necessary to handle the abrupt completion when calling PromiseResolve with a call to IfAbruptRejectPromise? So basically change AsyncFromSyncIteratorContinuation to use:

1. Let valueWrapper be PromiseResolve(%Promise%, « value »).
2. IfAbruptRejectPromise(valueWrapper, promiseCapability).

This was changed in a2647114b7f2d42b02e7c04c8c3a05787846f6e5. Maybe @MayaLekova can take a look?

[bakkot]

Is the difference between your two proposed fixes actually observable? At a quick glance:

• It looks like it's not possible for user code to obtain a reference to something inheriting from AsyncFromSyncIteratorPrototype (or at least shouldn't be); see https://github.com/tc39/proposal-realms/issues/84
• It looks like all consumers of such objects created by the spec do something along the lines of

Let value be ? Call(iteratorRecord.[[NextMethod]], etc).
Let result be ? Await(value).

(as in the runtime semantics for yield *)

which seems like it would make the difference immaterial, since there's no observable difference between returning an abrupt completion on the first of those two line and returning it on the second (I think, though there might be some number-of-ticks considerations). But I could easily be missing something; this is a very complicated part of the spec.

If the difference _is_ observable, a code snippet would be nice.

(cc @domenic)

[ljharb]

I think it was changed at my request; in general I’m trying to ensure we use the ? and ! prefixes as much as possible; however, certainly in this case it seems like it shouldn’t be there, even if it’s not observable.

[anba]

• It looks like it's not possible for user code to obtain a reference to something inheriting from AsyncFromSyncIteratorPrototype (or at least shouldn't be); see tc39/proposal-realms#84

Hah, at the end of that thread #1172 got rediscovered. :smile:

• It looks like all consumers of such objects created by the spec do something along the lines of
  [...]
  (I think, though there might be some number-of-ticks considerations).

Yes, it changes when the execution is resumed.

{
  async function f() {
    var p = Promise.resolve(0);
    Object.defineProperty(p, "constructor", {get() { throw "hi" }});
    for await (var x of [p]);
  }
  Promise.resolve(0)
         .then(() => print("tick 1"))
         .then(() => print("tick 2"))
         .then(() => print("tick 3"));
  f().catch(print);
}

Prints this with the current spec.

tick 1
hi
tick 2
tick 3

And this when IfAbruptRejectPromise is used:

tick 1
tick 2
hi
tick 3

But if nobody cares about that difference, maybe we could even change all %AsyncFromSyncIteratorPrototype% methods to no longer return rejected promises on abrupt completions?

[MayaLekova]

I agree that the current state of not propagating the possible abrupt completion up the call chain is wrong. Not sure which of the two fixes proposed is better though.

@anba Will the first version also be the output if the spec is changed so that all callers use ? AsyncFromSyncIteratorContinuation? I'll need to look closer into that, but on the first sight it seems this version won't change the behaviour for the implementors.

Regarding the difference, we had similar consideration about the change in await from which this issue originated. There was also an observable difference in behaviour introduced by it, in a not very common case. For now nobody seems to care though.

[anba]

Will the first version also be the output if the spec is changed so that all callers use ? AsyncFromSyncIteratorContinuation?

Yes, hi will be printed before tick 2 when the callers use ? AsyncFromSyncIteratorContinuation (and AsyncFromSyncIteratorContinuation continues to use ? PromiseResolve).

For now nobody seems to care though.

I could understand if someone argues that conceptually all %AsyncFromSyncIteratorPrototype% methods should always only return promise objects (and never throw an exception), basically as if the methods themselves were written as async functions. But I don't know if that's a sensible position. For me it's more important to be consistent in %AsyncFromSyncIteratorPrototype%: Either always directly propagate abrupt completions or always return with a rejected promise.

[zenparsing]

Either always directly propagate abrupt completions or always return with a rejected promise.

Agreed, and the current design for that operation is to return with a rejected promise.

[devsnek]

Prints this with the current spec.

just to clarify, with the current spec it's called with ! which means the example you provided technically shouldn't have run at all. in my engine for example, the current impl aborts the process.

i think this should be changed to use IfAbruptRejectPromise, not ?

[MayaLekova]

Thanks for the discussion! Please review the PR which implements the second fix proposed.

[s4san]

There are inconsistencies in how browsers handle abrupt completion (Firefox seems to have fixed this issue recently). Here is the result of running the above code on FF 67(Nightly), Chrome 75 (Canary) and Safari 12.0.3

| Browser | Output |
|--------- |--------------------------------- |
| FF 67 | tick 1 hi tick 2 tick 3 |
| Chrome 75 | tick 1 tick 2 tick 3 hi |
| Safari 12.0.3 | tick 1 tick 2 tick 3 hi |

The accompanying test could end up marking Chrome and Safari implementations as correct since it does not have 3 microtasks enqueued through then (thereby not validating if catch is handled before tick3)

[MayaLekova]

@s4san Thanks for pointing this out! It leads me to the following observations:

• We could make the test more robust by adding a 3rd tick before the catch, as the example from above does;
• Regarding Chrome compliance to the test, when running it locally with current d8 (which corresponds to Chrome 75) it seems that the implementation doesn't comply, so the test wouldn't mark it correct I believe. I'll look deeper asap and possibly comment on the test PR.

For reference, here's the error after running the test:
FAILED! Error: Test262Error: Expected [start, tick 1, tick 2] and [start, tick 1, tick 2, catch] to have the same contents