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

Editorial: Clarify the AsyncGenerator state machine #2413

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented May 24, 2021

AsyncGenerators are complicated, but at the root there's a relatively straightforward state machine. I find that it's difficult to see the shape of that machine with the current spec text, which routes every operation through AsyncGeneratorResumeNext.

I've rewritten all the logic here. The main reason I prefer this text is that each AO does one thing and returns, instead of kicking off a chain of self-recursive AOs which you have to follow for quite a while, keeping track of the state, before you figure out what's actually going to happen. For example, I think this PR makes it much more obvious that AsyncGeneratorYield might not return control to the rest of the program, and under what circumstances that happens. As a bonus, it makes AsyncGeneratorYield follow the pattern described in this comment, hopefully providing a path towards clarifying execution context juggling.

I don't believe there's any normative changes except for possibly, in certain cases, to the realm used for creating iterator result objects. (Edit: I've tweaked things to ensure it preserves the original, fairly strange behavior. I think we may want to change that behavior, but not as part of this PR.)

This PR has an incorrect ! assertion in callers of AsyncGeneratorAwaitReturn, corresponding to the same assertion in callers of AsyncGeneratorResumeNext described in #2412. I'll fix this PR up if that issue is resolved first. Otherwise it just leaves things in the same state.

Rather than trying to confirm that the logic is the same by comparing the two texts, I strongly suggest you read this gist where I've described the state machine, and confirm that both the original spec text and the text in this PR correspond to that state machine.

cc @jmdyck, I know you've spent a long time puzzling over this as well.

jmdyck
jmdyck previously requested changes May 25, 2021
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@jmdyck
Copy link
Collaborator

jmdyck commented May 25, 2021

Also, this PR undefines the following ids:

  • sec-asyncgeneratorreject
  • sec-asyncgeneratorresolve
  • sec-asyncgeneratorresumenext

(I don't know if there's an appropriate place to make them oldids.)

@bakkot bakkot changed the title Editorial (?): Clarify the AsyncGenerator state machine Editorial: Clarify the AsyncGenerator state machine Jun 5, 2021
@bakkot
Copy link
Contributor Author

bakkot commented Jun 5, 2021

Also, this PR undefines the following ids:

Yeah, I don't think there's a reasonable place to put them. They don't really correspond to any of the new operations - the whole point of the PR is that those previous operations were not a clean way of organizing the logic here, after all.

spec.html Outdated Show resolved Hide resolved
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelficarra and I are splitting up reviewing the current text for correspondence with the gist and reviewing the PR text for correspondence.

This lgtm is for correspondence with the current text.

The state machine in the gist reads correct to me. My understanding is as follows.

The current spec uses AsyncGeneratorResumeNext as a message pump that blocks suspends to wait for new messages. That is, it doesn't just resume next and continues execution, it generally pumps until it runs out of messages. This pump loop is awkwardly captured by self-recursive calls in the following states:

  • From executing, when a yield is evaluated, if the queue is not empty, there is a synchronous transition executing -> suspendedYield -> executing.
  • From completed, the queue is pumped and each promise is resolved with undefined until a return completion, in which case it goes into awaiting-return.

Put a third way, the current spec puts the focus on the message pump machinery, which is more confusing than putting the focus on the state machine machinery.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated

<emu-clause id="async-generator-await-return-fulfilled-functions" oldids="async-generator-resume-next-return-processor-fulfilled">
<h1>AsyncGeneratorAwaitReturn Fulfilled Functions</h1>
<p>An AsyncGeneratorAwaitReturn fulfilled function is an anonymous built-in function that is used as part of the AsyncGeneratorAwaitReturn specification device to unwrap promises passed in to the <emu-xref href="#sec-asyncgenerator-prototype-return" title></emu-xref> method. Each AsyncGeneratorAwaitReturn fulfilled function function has a [[Generator]] internal slot.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p>An AsyncGeneratorAwaitReturn fulfilled function is an anonymous built-in function that is used as part of the AsyncGeneratorAwaitReturn specification device to unwrap promises passed in to the <emu-xref href="#sec-asyncgenerator-prototype-return" title></emu-xref> method. Each AsyncGeneratorAwaitReturn fulfilled function function has a [[Generator]] internal slot.</p>
<p>An AsyncGeneratorAwaitReturn fulfilled function is an anonymous built-in function that is used as part of the AsyncGeneratorAwaitReturn specification device to unwrap promises passed in to the <emu-xref href="#sec-asyncgenerator-prototype-return" title></emu-xref> method. Each AsyncGeneratorAwaitReturn fulfilled function has a [[Generator]] internal slot.</p>

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with respect to correspondence to the state machine.

@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
@bakkot bakkot force-pushed the rewrite-asyncgenerator-state-machine branch from d17e46d to 18100ca Compare July 4, 2021 23:11
@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants