Normative: Close sync iterator when async wrapper yields rejection

[mhofman]

This updates Async-from-Sync Iterator Objects so that the async wrapper closes its sync iterator when the sync iterator yields a rejected promise as value. This also updates the async wrapper to close the sync iterator when throw is called on the wrapper but is missing on the sync iterator, and updates the rejection value in that case to a TypeError to reflect the contract violation. This aligns the async wrapper behavior to the yield* semantics.

Slides presented at TC39 plenary in January 2020.

Close on rejection

A rejected promise as value is transformed by the async wrapper into a rejection, which is considered by consumers of async iterators as a fatal failure of the iterator, and the consumer will not close the iterator in those cases. However, yielding a rejected promise as value is entirely valid for a sync iterator. The wrapper should adapt both expectations and explicitly close the sync iterator it holds when this situation arise.

Currently a sync iterator consumed by a for..await..of loop would not trigger the iterator's close when a rejected promise is yielded, but the equivalent for..of loop awaiting the result would. After this change, the iterator would be closed in both cases. Closes #1849

This change plumbs the sync iterator into AsyncFromSyncIteratorContinuation with instructions to close it on rejection, but not for return calls (as the iterator was already instructed to close), or if the iterator closed on its own (done === true).

Close on missing throw

If throw is missing on the sync iterator, the async wrapper currently simply rejects with the value given to throw. This deviates from the yield * behavior in 2 ways: the wrapped iterator is not closed, and the rejection value not a TypeError to indicate the contract was broken. This updates fixes both differences by closing the iterator, and throwing a new TypeError instance instead of the value provided to throw.

Since the spec never calls throw on an iterator on its own (it only ever forwards it), and that the async wrapper is never exposed to the program, the only way to observe this async wrapper behavior is through a program calling yield * with a sync iterator from an async generator, and explicitly call throw on that async iterator.

[jridgewell]

I think there's one more case with step 5 and 6:

5. Let valueWrapper be PromiseResolve(%Promise%, value).
6. IfAbruptRejectPromise(valueWrapper, promiseCapability).

The iterator is returning a value, which may throw when we try to coerce into a native promise. Eg,

function* gen() {
  try {
    const p = Promise.resolve('FAIL');
    Object.defineProperty(p, 'constructor', {
      get() {
        throw new Error('foo');
      }
    });
    yield p;
  } finally {
    console.log('PASS');
  }
}

(async () => {
  for await (const v of gen()) {
    console.log(v);
  }
})()

This should call the finally, but doesn't.

[jridgewell]

Wouldn't you need to close here? Can you recover from a recovery?

[bakkot]

Good question. I find it hard to know what the "right" answer is for throw, since it doesn't come up as much (the user has to call it explicitly), but I think you're right. Compare, for example,

function* count() {
  try {
    for (let i = 0; i < 10; ++i) {
      try {
        yield i;
      } catch (e) {
        console.log("caught and suppressed", e);
      }
    }
  } finally {
    console.log("time to clean up");
  }
}

function* wrap(inner) {
  yield* inner;
}

let iterable = wrap(count());
for (let item of iterable) {
  console.log(item);
  console.log("throw returned", iterable.throw("throwing here")); // note that this advances the iterator
  if (item >= 4) break;
}

This goes through three iterations of the loop, printing "caught and suppressed throwing here" in each one, and then on the last iteration, which breaks, also prints "time to clean up".

(The call to wrap in this example has no observable effects, vs just iterable = count() which is my point - calling .throw on this wrapper delegates it to the inner iterator, which suppresses the exception, so the loop continues and can later call .return.)

---

I'm not at all clear on when you'd ever want to do call throw - I think it was copied from Python, which uses specific kinds of exceptions for more general signals in a way that JS does not, but this was before my time and I haven't reviewed all the relevant notes. But if we assume the analogy above is reasonable, then I agree with @jridgewell.

[mhofman]

Good catch, I totally forgot about throw being able to recover. And I agree, letting the sync iterator decide is the right approach. Good thing is, since we now check the done value before installing the close on rejection logic, just switching the boolean to true here should be sufficient.

Which begs the question, should return be handled similarly? It seems that the result of return can similarly continue a yield * from my reading of the steps (7.c.viii.)

[bakkot]

Which begs the question, should return be handled similarly?

Huh. I guess so, looking at it, though I think this matters less - as these slides say, failure to stop iterating when return is called is probably a bug in the contract between the iterator and the loop.

(It would allow dropping the closeIteratorOnRejection parameter entirely, which is nice.)

[mhofman]

Based on

• In short: any abrupt completion of the loop.
• Normal completion should not call the method; in
  that case the iterator itself decided to close

I'm willing to say, let's close if the sync iterator doesn't believe it's done, but the consumer of the wrapper considers the async iterator is buggy, regardless of what caused the iterator result to be yielded.

[mhofman]

except to forward on explicit calls to throw from the user

Yeah, and this is actually another inconsistency of this wrapper. If sync iterator doesn't have a throw method, the wrapper doesn't fallback to .return like yield * would.

Here we would be calling it twice. That seems like it would violate the expectation that it gets called exactly once. It's also not clear what benefit it would have: the language calls return to say "I'm done with this", but we've already done that the first time we called return, and it's not like we're going to be any more done just because the promise rejected.

Yeah I think I agree.

@bakkot, what do you think of still calling .return if .throw returns a rejected promise as value with done: false, but not calling .return a second time if .return was already called, regardless if it returned done: false or not. In that case I can reintroduce the boolean flag and name it something like "returnAlreadyCalled".

Also, do you think we should add a .return fallback in the wrapper's .throw?

[bakkot]

I don't have much intuition for how throw is supposed to work, to be honest.

[mhofman]

I don't have much intuition for how throw is supposed to work, to be honest.

Step 7.b.3 of yield * falls back to calling .return if throw is undefined on the iterator. The problem is that the wrapper has a throw on its prototype regardless of the shape of the sync iterator, so yield * would call the wrapper's .throw, which would simply reject because it can't find a .throw on the sync iterator, and not call .return like yield * would have if the wrapper lacked a .throw.

[bakkot]

Yeah, I guess the analogy to yield* is compelling. I'm on board with both

• if throw is called on the outer iterator and the inner iterator lacks throw, fall back to return
• if throw is called on the outer iterator and the inner iterator has throw and it returns { done: false, value: Promise.reject() }, call return on the inner iterator.

[jridgewell]

@bakkot's suggestion SGTM.

[mhofman]

The iterator is returning a value, which may throw when we try to coerce into a native promise.

In principle I'd say yes, but this will complicate things quite a bit, and arguably we have an iterator yielding bogus values

[bakkot]

In principle I'd say yes, but this will complicate things quite a bit

Is it not sufficient to add 1. If _valueWrapper_ is an abrupt completion, set _valueWrapper_ to IteratorClose(_syncIteratorRecord_, _valueWrapper_) before step 6? That's not very much complexity.

[mhofman]

But only if closeIteratorOnRejection and not done, right ?

[bakkot]

Sorry, right.

[mhofman]

PTAL @bakkot @jridgewell .

I decided to ignore the operation type when closing the iterator and solely rely on the done value.
If the sync iterator decides it isn't actually done on return, it will get another closing attempt from IteratorClose, but that's fine by me since it's a buggy behavior in the first place on behalf of the sync iterator.

[mhofman]

@bakkot @jridgewell, PTAL

I believe this now specifies the behavior we discussed in #2600 (comment)

We could consider rejecting with TypeError for missing throw like yield *.

This is yet another discrepancy I found, but I'm less sure if we should fix it.

[bakkot]

LGTM other than comment.

[jmdyck]

Looks okay to me.

Note that AsyncFromSyncIteratorContinuation's new _syncIteratorRecord_ parameter would be an Iterator Record in PR #2591, so whichever lands second should update.

[vadzim]

@mhofman

First of all thank you a lot for your work. Recent Chrome and Node already have this fix.

I've just realized that just closing sync iterator causes different behavior with try/catch within async and sync generators

like

void async function () {
  try {
    for await (const num of function*() {
      try {
        yield 1;
        yield Promise.reject(2);
        yield 3;
      } catch (e) {
        console.log("called catch", e);
        throw e;
      } finally {
        console.log("called finally");
      }
    }()) {
      console.log(num);
    }
  } catch (e) {
    console.log("caught", e);
  }
}()
// 1
// called finally
// caught 2

and

void async function () {
  try {
    for await (const num of /*->*/async/*<- the only difference*/ function*() {
      try {
        yield 1;
        yield Promise.reject(2);
        yield 3;
      } catch (e) {
        console.log("called catch", e);
        throw e;
      } finally {
        console.log("called finally");
      }
    }()) {
      console.log(num);
    }
  } catch (e) {
    console.log("caught", e);
  }
}()
// 1
// called catch 2
// called finally
// caught 2

In particular that means that async iterator can catch the error, handle it and continue to run while sync iterator cannot.

I would personally prefer that AsyncFromSyncIteratorContinuation makes sync iterator behavior exactly the same as async one and not to explain my grandchild why even recent JS features are so damn strange. But I'm not really sure if it's worth to spend someone's time to fix that.

[mhofman]

In particular that means that async iterator can catch the error, handle it and continue to run while sync iterator cannot.

I would personally prefer that AsyncFromSyncIteratorContinuation makes sync iterator behavior exactly the same as async one and not to explain my grandchild why even recent JS features are so damn strange. But I'm not really sure if it's worth to spend someone's time to fix that.

This is a potentially more complicated problem. In the case of the async generator, the catch is triggered because the yield does an implicit await of the yielded value. The spec currently does not ever invoke throw on its own on an iterator: it only passes it through in the case of yield * or when a async-from-sync iterator has its throw called.

I do not recall if this was explicitly discussed in plenary, but technically we could attempt to invoke the throw of the sync iterator if it exists in the onRejected handler. This would be another normative change.

[vadzim]

technically we could attempt to invoke the throw of the sync iterator if it exists in the onRejected handler

yep, sync-to-async wrapper already awaits the sync generator's yielded values simulating the implicit awaits within an async generator.
just it does not simulate throwing an exception.

[sosukesuzuki]

This normative change has been implemented in JSC:

• [JSC] AsyncFromSyncIterator should close its sync-iterator when throw is null or undefined WebKit/WebKit#39876
• [JSC] AsyncFromSyncIterator should close its sync-iterator when next/throw yields rejected promise WebKit/WebKit#40314

changes addressed

[vadzim]

Sorry, guys
I'm just not sure
Do those PRs also resolve the issue described in #2600 (comment) ?
@mhofman
@sosukesuzuki

[bakkot]

@vadzim No, no one has proposed to change that behavior (and I would be opposed to changing it - that looks like the right behavior to me).

[mhofman]

Do those PRs also resolve the issue described in #2600 (comment) ?

It does not.

According to #1849 (comment), there are opinions to not have the synthetic throw behavior you propose. You could open a new issue, but that change would be harder to justify.

I am personally ambivalent, but it's not really clear what the behavior should be if throw exists, and e.g. the catch clause doesn't retrow but instead yields more values.