yield * in an async iterator should not call .throw when the inner iterator produces a rejected promise

[bakkot]

Consider the following horrible program:

'use strict';
var console = console || { log: print };

let inner = {
  [Symbol.asyncIterator]: () => ({
    next() {
      return Promise.resolve({ done: false, value: Promise.reject('rejection') });
    },
    throw(e) {
      console.log('throw called with', e);
      throw e;
    },
    return(e) {
      console.log('return called with', e);
      return {};
    },
  }),
};

async function* outer() {
  yield* inner;
}

(async () => {
  for await (let x of outer()) {
    console.log(x);
  }
})().catch(e => {
  console.log('threw', e);
});

Per spec, this will print "throw called with 'rejection'". Specifically, the evaluation semantics for yield* step 7.a.vi will invoke AsyncGeneratorYield passing Promise.reject('rejection'), which will then be Await'd, which will throw, causing the received alias to hold a throw completion. The following machinery then acts as if .throw had been explicitly called on outer, which is the only other way that received can hold a throw completion - the behavior in that case being to invoke .throw on inner.

I think this possibility was simply overlooked during the spec'ing of async generators. This is the only place in the entire spec that .throw is called without a user explicitly invoking it at some point. (Previously I'd thought there were no such places.) And the aliases certainly imply that the algorithm is assuming that a throw completion must have come from the user, rather than from the iterator itself.

This behavior seems wrong to me. Normally when consuming an iterator the spec will either a.) determine that the iterator itself is broken (e.g. it threw), in which case it is assumed to have done its own cleanup and the spec will not make further calls to any of its methods, or b.) determine that the iterator is fine but for some reason the consumer needs to exit early, in which case the spec will call .return (not .throw) to give the iterator a chance to clean up.

---

I am inclined to treat this as in (b). Nothing in the spec prevents custom async iterators from returning promises - see the snippet in this comment. So I think we ought to treat the await iterResult.value as being something the outer iterator is doing, rather than a violation of the iteration protocol, so that if this await happens to throw then the inner iterator should be closed with .return.

I could probably be persuaded to treat this as in (a), i.e. to say that this is a violation of the iteration protocol and so the spec need not clean up the inner iterator.

The current behavior seems bad, though.

For comparison,

for await (let item of inner) {
  yield item;
}

will call .return on inner, in the above example. I would like yield* to match this case.

---

For the snippet above, JavaScriptCore and V8 implement the spec (they print "throw called with rejection" and then "threw rejection"). SpiderMonkey, Graal, and XS only print "threw rejection" (they also don't close the inner iterator).

[bathos]

I’d figured the reasoning here was that even though next() produced a normal completion for a valid iterator result, the promise represents the eventual “actual” completion. It seems consistent with how Promises are “flattened” elsewhere.

I was confused by some other things here though which seemed to be saying that "throw" is only called in this one scenario, which isn’t true. I might be misunderstanding, but I mean these bits sounded like they concerned not only that value-rejects case (which is unique) but also the “directly produced a throw completion” cases (which aren’t):

The following machinery then acts as if .throw had been explicitly called on outer, which is the only other way that received can hold a throw completion.

[Normally if the spec determines] that the iterator itself is broken (e.g. it threw) [...] it is assumed to have done its own cleanup and the spec will not make further calls to any of its methods.

The received.[[Type]] value will be throw if next() returns a throw completion, not just when an async generator returns a normal completion whose value is an iterator result object whose eventual-value is rejected. A "throw" method is called if present for both sync and async cases if next() throws.

If this weren’t the case, the resumption model would end up “backwards” for throw completions uniquely: catch blocks wouldn’t be entered when yield * expressions evaluate to throw completions. That generators can be resumed with the the full range of “observable” completion types (normal, return, or throw) at their yield points “equally” is pretty central to the control flow model and (as far as I can tell) it’s those steps which make that happen, right?

[bakkot]

I’d figured the reasoning here was that even though next() produced a normal completion for a valid iterator result, the promise represents the eventual “actual” completion. It seems consistent with how Promises are “flattened” elsewhere.

Even if that were true, the consistent behavior would be to not call any methods, on the assumption that the generator will have already done its own cleanup in that case, in the same way that the spec does not call .throw or .return if a sync generator throws or returns an iterator result object with a throwy done getter.

(But I don't think the view that the promise here represents the eventual “actual” completion is consistent with the rest of the spec; contrast the behavior of for await, in the snippet I linked.)

I was confused by some other things here though which seemed to be saying that "throw" is only called in this one scenario, which isn’t true.

In what other scenario does .throw get called without user code directly invoking it at some point?

The received.[[Type]] value will be throw if next() returns a throw completion [...] A "throw" method is called if present for both sync and async cases if next() throws.

That does not match my reading. Step 7.a.i of the yield * semantics: "Let innerResult be ? Call(iteratorRecord.[[NextMethod]], ...)". That will immediately propagate any throw completions from the invocation of next(). (In an async generator, step 7.a.ii will also propagate any throw completions represented by returned rejected promises.)

For a sync generator, as far as I can tell, the only way that received.[[Type] can hold a throw completion is if GeneratorYield produces a throw completion, and the only way that can happen is if .throw is called explicitly (or transitively via another wrapping generator doing yield *) while the generator is paused.

Try it yourself:

let inner = {
  [Symbol.iterator]: () => ({
    next() {
      throw 0;
    },
    throw(e) {
      console.log('throw called with', e);
      throw e;
    },
    return(e) {
      console.log('return called with', e);
      return {};
    },
  }),
};

function* outer() {
  yield* inner;
}

for (let item of outer()) {
  console.log(item);
}

No engine I have on hand will invoke the "throw" method in this code.

That generators can be resumed with the the full range of “observable” completion types (normal, return, or throw) at their yield points “equally” is pretty central to the control flow model and (as far as I can tell) it’s those steps which make that happen, right?

It is those steps which make that happen, yes, but the point I am making is that if a generator has not been resumed with a throw completion then .throw should not be called. Here, even though the outer generator is resumed with .next, the inner generator still gets resumed with .throw, which is inconsistent with the rest of the spec.

[bathos]

Thanks for clearing that up! I see it now.

The/a reason I had the wrong idea about this might be related to the issue you’ve described (in a general way: “don’t fuss with the ‘inners’ when it’s not needed”). I’ve often hit (what I now see is) step 7.b.iii.6 when using generators for “manual” control flow because it’s easy to reach when yield * is paired with most “regular” iterables (as they don’t implement “throw”). So if the generator happens to be “in the middle of” a yield * typicalIterable when the next input should be a throw completion, step 7.b.iii.6 swallows the real error value and replaces it with a new one. In at least one engine the error message produced echoed the note above there about the absence of that inner throw constituting a “protocol violation”. I mistook that for a more general requirement / the mechanism itself.

I think you’re correct about the async gen case being incorrect and am left wondering if the error “replacement” at 7.b.iii.6 is unnecessary for similar reasons; if yield * [ 1, 2, 3 ]’s array iterator is suspended when we resume the generator around it, is there really any good reason it “must” implement throw / discard the input error? It closes regardless and the absence of throw seems to strongly imply / state outright “I don’t have or need ‘throw steps’”.

[bakkot]

if yield * [ 1, 2, 3 ]’s array iterator is suspended when we resume the generator around it, is there really any good reason it “must” implement throw / discard the input error? It closes regardless and the absence of throw seems to strongly imply / state outright “I don’t have or need ‘throw steps’”.

Yeah, I also find that behavior surprising. It seems more natural to propagate the original exception instead of clobbering it - especially given that the behavior for a missing .return is to propagate the original return completion.

That also matches how I think about these handlers - an implementation of .return is roughly analogous to a finally block, so if you don't have one when a return; is triggered the return completion will just propagate, which is what actually happens; similarly, I think of an implementation of .throw as being roughly analogous to a catch block, so it would make sense that if you don't have one when an exception is triggered the exception will propagate (after first triggering any finally blocks i.e. any .return handlers), but instead the exception gets replaced with a different one, which is weird.

[bakkot]

On the other hand, if you tried to call .throw directly on an iterator which didn't have it at all you would get a TypeError, not the original exception, so maybe the idea is to keep consistency with that.

[bathos]

Yeah, that occurred to me too as the likely rationale. In practice for me it’s meant adding generator wrappers around things that could have directly delegated to native iterables if not for that step, so I tend to think it’s creating rather than solving a problem (now that I understand what’s really happening there). I don’t know if that’s always the case.

(But this is something apart from the issue you’ve described anyway — sorry to sidetrack & thanks for the new clarity.)

[bakkot]

Following up here: #2819 got consensus at the July 2022 plenary meeting. We're waiting on implementations before landing that PR, at which point this can be closed.