Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec Issue: IteratorStep returns a promise for async iterators, leading to an infinite loop #33

Closed
mgaudet opened this issue Oct 14, 2022 · 2 comments · Fixed by #44 or #45
Closed
Labels
bug Something isn't working

Comments

@mgaudet
Copy link

mgaudet commented Oct 14, 2022

In the published spec, Step 3.j.ii.3 and 4 are the termination condition of the iteration:

  • Step 3.j.ii.3 "Let next be ? Await(IteratorStep(iteratorRecord))."
  • Step 3.j.ii.4 "If next is false, then..."

The problem is that IteratorStep doesn't properly flag termination on an async iterator; the return value of the IteratorNext in async iteration is a Promise, which doesn't have a "done" property, and so we always get the promise object from IteratorStep.

I think the patch is actually relatively simple; Step 3.j.ii.4 should be expanded into the steps which have the effect of "if next.done is false:"

@bakkot
Copy link
Contributor

bakkot commented Oct 14, 2022

Good catch. The fix is a little more complicated than your suggestion, though. IteratorStep isn't usable at all with async iterators; it performs IteratorComplete on the result of the next method, which assumes the return value of that method is an iterator result, not a promise for one.

I think instead we want to invoke the next method directly, like for await does (which is currently one of the only consumers of the async iteration protocol):

1. Let nextResult be ? Call(iteratorRecord.[[NextMethod]], iteratorRecord.[[Iterator]]).
1. Set nextResult to ? Await(nextResult).
1. If nextResult is not an Object, throw a TypeError exception.
1. Let done be ? IteratorComplete(nextResult).
1. If done is true,
  1. Perform ? Set(A, "length", 𝔽(k), true).
  1. Return Completion Record { [[Type]]: return, [[Value]]: A, [[Target]]: empty }.
1. Let nextValue be ? IteratorValue(nextResult).
[etc]

Alternatively, I suppose we could have IteratorStep take an option "async" flag which would cause it to do the awaiting itself. Something to worry about during the stage 4 PR, possibly.

@mgaudet
Copy link
Author

mgaudet commented Mar 29, 2023

Note: I'm preparing to ship Array.fromAsync. It will ship using the above spec-patch semantics.

@js-choi js-choi mentioned this issue Nov 1, 2023
5 tasks
js-choi added a commit that referenced this issue Dec 2, 2023
Fixes #33.
Prevents an infinite loop caused by IteratorStep(iteratorRecord) not actually flagging termination when IteratorRecord is async. Instead, we will directly call iteratorStep.[[NextMethod]], like how `for await` does.

Co-Authored-By: Kevin Gibbons <[email protected]>
js-choi added a commit that referenced this issue Dec 2, 2023
Fixes #33.
Prevents an infinite loop caused by IteratorStep(iteratorRecord) not actually flagging termination when IteratorRecord is async. Instead, we will directly call iteratorStep.[[NextMethod]], like how `for await` does.

Co-Authored-By: Kevin Gibbons <[email protected]>
@js-choi js-choi added the bug Something isn't working label Dec 2, 2023
js-choi added a commit that referenced this issue Dec 21, 2023
Fixes #33.
Prevents an infinite loop caused by IteratorStep(iteratorRecord) not actually flagging termination when IteratorRecord is async. Instead, we will directly call iteratorStep.[[NextMethod]], like how `for await` does.

Co-authored-by: Kevin Gibbons <[email protected]>
js-choi added a commit that referenced this issue Dec 24, 2023
Finish #44’s change, which fixes #33.

Co-Authored-By: Kevin Gibbons <[email protected]>
#44 renamed _next_ to _nextResult_ but one reference was left unchanged. Spotted by @trflynn89.
js-choi added a commit that referenced this issue Dec 24, 2023
Finish #44’s change, which fixes #33.

Co-Authored-By: Kevin Gibbons <[email protected]>
#44 renamed _next_ to _nextResult_ but one reference was left unchanged. Spotted by @trflynn89.
js-choi added a commit that referenced this issue Dec 27, 2023
Finish #44’s change, which fixes #33.

Co-Authored-By: Kevin Gibbons <[email protected]>
#44 renamed _next_ to _nextResult_ but one reference was left unchanged. Spotted by @trflynn89.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants