finally is not called when asynchronously iterating over synchronous generator which yields rejected promise

[vadzim]

Description: Finally block in sync generator in try-finally statement is ignored and is not executed if that generator yields rejected promise and is iterated over with for-await-of loop.

void async function() {
	try {
		for await (const x of function*() {
			try { yield Promise.reject("reject") }
			finally { console.log("finally after reject") }
		}()) {
			console.log(await x)
		}
	} catch (e) {
		console.log(e)
	}
}()

eshost Output:

$ eshost -s -x 'void async function() {
	try {
		for await (const x of function*() {
			try { yield Promise.reject("reject") }
			finally { console.log("finally after reject") }
		}()) {
			console.log(await x)
		}
	} catch (e) {
		console.log(e)
	}
}()'
#### sm, v8
reject

I expect the following output:

finally after reject
reject

If the generator is async then finally block is executed as expected.

If for-await-of loop is transpiled with babel then finally block is executed as expected.

There is a comment about that bug and spec compliance in firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1610315#c1

Update.
This bug causes the next two code fragments to behave differently:
1.

   for (const _x of function*() {
      try { yield Promise.reject("reject") }
      finally { console.log("finally after reject") }
   }()){
      const x = await _x
      ....
   }

   for await (const x of function*() {
      try { yield Promise.reject("reject") }
      finally { console.log("finally after reject") }
   }()){
      ...
   }

The first loop executes finally block, but the second one doesn't, though both of them resolves promises before assigning them to x.

[vadzim]

It seems that for-await-of loop awaits for yielded promises in its body and throws errors from rejected promises, but does not call .return() method of the generator in that case.

[vadzim]

I'm not sure, but may be it's connected with #1765 in some way.

[ljharb]

Are you suggesting that the spec needs fixing here? If so, what change would you suggest?

[devsnek]

'use strict';

function* inner() {
  try {
    yield Promise.reject('reject');
  } finally {
    print('A');
  }
}

async function outer() {
  try {
    for await (const x of inner()) {
    }
  } catch (e) {
    print('B', e);
  }
}

outer();

┌────────────────┬───────────────────────────┐
│ ChakraCore     │                           │
│                │ SyntaxError: Expected '(' │
├────────────────┼───────────────────────────┤
│ engine262      │ B reject                  │
│ engine262+     │                           │
│ JavaScriptCore │                           │
│ SpiderMonkey   │                           │
│ V8             │                           │
│ XS             │                           │
├────────────────┼───────────────────────────┤
│ GraalJS        │ A                         │
│ QuickJS        │ B reject                  │
└────────────────┴───────────────────────────┘

As far as I can tell, just printing C reject is correct.

[nicolo-ribaudo]

Note that the synchronous version of that code calls iterator.throw():

function* inner() {
  try {
    throw 'reject';
  } finally {
    print('A');
  }
}

function outer() {
  try {
    for (const x of inner()) {
      print('B', x);
    }
  } catch (e) {
    print('C', e);
  }
}

outer();

Logs A and C reject.

[devsnek]

@nicolo-ribaudo it does not call iterator.throw. inner().next() throws (it doesn't return a normal completion), and the loop is left entirely.

[nicolo-ribaudo]

Oh you are right 😅
This case doesn't have a synchronous equivalent because the loop throws after calling .next() but before entering the loop body.

[vadzim]

@ljharb yes, I suggest the spec needs fixing.

In the next attachment 5 of 6 generators are opened and then are closed by the loop:
https://bug1610315.bmoattachments.org/attachment.cgi?id=9121851

but 1 of 6 is not.

It seems very confusing that execution of finally block depends on very specific case.

I do not expect that for-[await]-of loop closes generators correctly in mostly all cases, I do expect that loop closes generator every time it has open that generator.

[devsnek]

Okay looking through a bit more, this is definitely working as intended.

When an exception is caused by calling into the iterator protocol (iterator.next(), for example), the spec assumes the protocol implementation is in an exceptional state and doesn't call back into it again. This includes asyncIterator.next() returning a rejected promise.

An argument could be made that AsyncFromSyncIteratorPrototype should not unwrap the promise, but it would be a breaking change at this point.

[vadzim]

I believe the code

   for await (const x of function*() {
      try { yield Promise.reject("reject") }
      finally { console.log("finally after reject") }
   }()){
      ...
   }

should be equivalent of

   for (const _x of function*() {
      try { yield Promise.reject("reject") }
      finally { console.log("finally after reject") }
   }()){
      const x = await _x
      ....
   }

But actually the second one does execute finally block, and the first one doesn't.

It's not easy to understand why it should behave in that way.

[vadzim]

@devsnek yes, it seems that the problem is on AsyncFromSyncIterator side.

Is it actually a breaking change for the code in real world?

I do not believe that there exists a code which relies on such a behavior, when finally block should not be executed in some generator if it is iterated in async loop. Doesn't it?

[vadzim]

@devsnek

An argument could be made that AsyncFromSyncIteratorPrototype should not unwrap the promise, but it would be a breaking change at this point.

It should unwrap, but it should become more clever proxy and should call return on iterator if the promise is rejected.

async function* AsyncFromSyncIterator(iterator) {
  for (const x of iterator) {
    yield await x
  }
}

does the trick and guarantees that all finally blocks will be correctly executed.

[vadzim]

Another non-intuitive behavior of the current spec:

function* generator() {
   try {
      yield 2
      yield Promise.reject(3)
   } finally {
      print("finally")
   }
}

async function* asyncGenerator() {
   // 1)
   for (const x of generator()) yield x
   // 2)
   yield* generator()
}

The 1) closes generator and executes finally block, the 2) does not.

[vadzim]

@ljharb So what do you think that AsyncFromSyncIterator should be responsible of closing syncronous iterator if rejected promise is yielded?
I think it's wrong that executing finally blocks depends on async or sync for-of loop is consuming the generator.

[ljharb]

#1849 (comment) seems to argue persuasively that everything is working as intended, and can’t likely change even if it wasn’t.

[jridgewell]

I think it's working as intended, but it's still wrong. Given the matrix of sync/async iterators and sync/async iteration, we get weird results if we keep this as is:

Test Cases

'use strict';

// Iterators
function* syncIteratorYield() {
  try {
    yield (Promise.reject('reject'));
  } finally {
    print('A');
  }
}

function* syncIteratorThrow() {
  try {
    throw 'reject'
  } finally {
    print('A');
  }
}

async function* asyncIteratorYield() {
  try {
    yield (Promise.reject('reject'));
  } finally {
    print('A');
  }
}

async function* asyncIteratorThrow() {
  try {
    throw 'reject'
  } finally {
    print('A');
  }
}

// For Of Loops
async function syncYield() {
  try {
    for (const x of syncIteratorYield()) {
    }
  } catch (e) {
    print('B', e);
  }
}

async function syncThrow() {
  try {
    for (const x of syncIteratorThrow()) {
    }
  } catch (e) {
    print('B', e);
  }
}

async function asyncSyncYield() {
  try {
    for await (const x of syncIteratorYield()) {
    }
  } catch (e) {
    print('B', e);
  }
}

async function asyncSyncThrow() {
  try {
    for await (const x of syncIteratorThrow()) {
    }
  } catch (e) {
    print('B', e);
  }
}

async function asyncAsyncYield() {
  try {
    for await (const x of asyncIteratorYield()) {
    }
  } catch (e) {
    print('B', e);
  }
}

async function asyncAsyncThrow() {
  try {
    for await (const x of asyncIteratorThrow()) {
    }
  } catch (e) {
    print('B', e);
  }
}

(async function run() {
  print('sync iteration of sync iterator, yield rejection');
  try { await syncYield(); } catch (e) {}
  print('');

  print('sync iteration of sync iterator, throw');
  try { await syncThrow(); } catch (e) {}
  print('');

  print('async iteration of sync iterator, yield rejection');
  try { await asyncSyncYield(); } catch (e) {}
  print('');

  print('async iteration of sync iterator, throw');
  try { await asyncSyncThrow(); } catch (e) {}
  print('');

  print('async iteration of async iterator, yield rejection');
  try { await asyncAsyncYield(); } catch (e) {}
  print('');

  print('async iteration of async iterator, throw');
  try { await asyncAsyncThrow(); } catch (e) {}
})()

#### chakra

SyntaxError: Syntax error
(note: Chakra can't handle an async generator or for-await-of)

#### javascriptcore, spidermonkey, v8
sync iteration of sync iterator, yield rejection
A

sync iteration of sync iterator, throw
A
B reject

async iteration of sync iterator, yield rejection
B reject

async iteration of sync iterator, throw
A
B reject

async iteration of async iterator, yield rejection
A
B reject

async iteration of async iterator, throw
A
B reject

I expect "sync iteration of sync iterator, yield rejection" to be different, because yielding a rejected promise in a sync-sync iteration isn't an exception case. But that the other 5 cases (4, if you want to throw out the sync-sync throw) don't align is unexpected.

[ljharb]

In that case, a needs-consensus PR would be the next step to advance the conversation.

[jridgewell]

I see two possible changes here:

1. Make sync-consuming async iteration ignore promise yield values
   • This is probably a huge breaking change, and not web compatible.
   • This is the breaking change @devsnek mentions in finally is not called when asynchronously iterating over synchronous generator which yields rejected promise #1849 (comment)
2. Make sync-consuming async iteration call it.return() in abrupt completion and yielded promise rejection
   • This could be breaking, but is likely fine for web compat.

I think 2 only requires changes to https://tc39.es/ecma262/#sec-asyncfromsynciteratorcontinuation, specifically Step 10 needs to pass capability.[[Reject]] as the 3rd param.

[vadzim]

@domenic I guess you're one of authors of async iteration proposal.
Could you please explain why there exists that option to leave generator unclosed?
Is it by some intention or is it by mistake? What is the reason if it's by intention?

[mhofman]

I just came up on this issue today, glad to know I'm not the only one who finds this behavior surprising

think 2 only requires changes to https://tc39.es/ecma262/#sec-asyncfromsynciteratorcontinuation, specifically Step 10 needs to pass capability.[[Reject]] as the 3rd param.

I was thinking that what needed to be passed as 3rd param was specific steps to close the iterator before passing through the wrapper rejection to the promise capability. When an iterator consumer gets a rejection from next, it will no longer interact with the async iterable, including not close it. That means the async from sync iterator object needs to do the cleanup, and close the sync iterator before rejecting.

[vadzim]

@mhofman Wow, thanks for fixing this

[mhofman]

Thank @bakkot for taking it over the line, and @ioannad and @ptomato for the tests!

[vadzim]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for-await...of#iterating_over_sync_iterables_and_generators
needs update after fixing. just a reminder for myself

[Josh-Cena]

And https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/fromAsync#no_error_handling_for_sync_iterables too. I'm surprised that https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#error_handling doesn't mention this case—oversight on my side. @vadzim if you want to champion the MDN work (based on your comment), you could start by sending an issue after the PR gets merged, because it would probably need browser compatibility data too.