Skip to content

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

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Closed
mgaudet opened this issue Oct 14, 2022 · 2 comments · Fixed by #44 or #45
Closed

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

mgaudet opened this issue Oct 14, 2022 · 2 comments · Fixed by #44 or #45
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 •
edited
Loading

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
Open
5 tasks
js-choi added a commit that referenced this issue Dec 2, 2023
@js-choi @bakkot
fix: Replace IteratorStep with calling
0c3f841 
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 <bakkot@gmail.com>
js-choi added a commit that referenced this issue Dec 2, 2023
@js-choi @bakkot
fix: Replace IteratorStep with NextMethod
3312edc 
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 <bakkot@gmail.com>
@js-choi js-choi mentioned this issue Dec 2, 2023
Merged
@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
@js-choi @bakkot
fix: Replace IteratorStep with NextMethod (#44)
689d0d4 
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 <bakkot@gmail.com>
js-choi added a commit that referenced this issue Dec 24, 2023
@js-choi
fix: Rename _next_ reference to _nextResult_
56ee3c2 
Finish #44’s change, which fixes #33.

Co-Authored-By: Kevin Gibbons <bakkot@gmail.com>
#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
@js-choi
fix: Rename _next_ reference to _nextResult_
6f14788 
Finish #44’s change, which fixes #33.

Co-Authored-By: Kevin Gibbons <bakkot@gmail.com>
#44 renamed _next_ to _nextResult_ but one reference was left unchanged. Spotted by @trflynn89.
@js-choi js-choi mentioned this issue Dec 24, 2023
Merged
js-choi added a commit that referenced this issue Dec 27, 2023
@js-choi
fix: Rename _next_ reference to _nextResult_ (#45)
73ca045 
Finish #44’s change, which fixes #33.

Co-Authored-By: Kevin Gibbons <bakkot@gmail.com>
#44 renamed _next_ to _nextResult_ but one reference was left unchanged. Spotted by @trflynn89.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 27, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
3 participants
@js-choi @bakkot @mgaudet