Skip to content
This repository has been archived by the owner on Aug 29, 2021. It is now read-only.

Revisit suspending the running execution context for modules, fixes #143 #175

Closed

Conversation

codehag
Copy link
Collaborator

@codehag codehag commented May 12, 2021

Tries to address #143 -- my reading of the spec is that, we should not be suspending the execution context in the case of an async module. The async module context plays the same role as the copy of the running context, asyncContext does in AsyncFunctionStart, and the running context in that case is not suspended. I believe it would be wrong to suspend in the case of modules, as execution should continue and the module context itself should be treated as a promise from my understanding.

@codehag codehag requested review from Ms2ger and guybedford May 12, 2021 12:19
@codehag codehag changed the title Revisit suspending the running execution context for modules Revisit suspending the running execution context for modules, fixes #143 May 12, 2021
@Ms2ger Ms2ger removed their request for review May 12, 2021 12:32
@Ms2ger
Copy link
Collaborator

Ms2ger commented May 12, 2021

This is the part of the spec that I always understood least, so I'm hoping @guybedford understands it :)

@guybedford
Copy link
Collaborator

I'm also not 100% clear on this, but there is no Suspend context call in AsyncBlockStart which makes me think we may still need it here since we definitely do want to suspend the currently running execution context in both cases?

Perhaps we can get another reviewer on this.

@guybedford
Copy link
Collaborator

I guess the question is, does this line of AsyncFunctionStart:

5. Push asyncContext onto the execution context stack; asyncContext is now the running execution context.

effectively take the place of the suspend the running execution context line?

I do see examples in the spec though of that line being called after suspending the current execution context, for example see https://tc39.es/ecma262/#await-fulfilled where in step 4 the current context is suspended just before calling a push.

@codehag
Copy link
Collaborator Author

codehag commented May 13, 2021

That was my reading of the spec, but it might be wrong.

@littledan who would be the best person to ask about this?

@codehag
Copy link
Collaborator Author

codehag commented May 13, 2021

After chatting with @guybedford it looks like the existing spec is correct.

1. <del>Let _moduleContext_ be _module_.[[Context]].</del>
1. <del>Push _moduleContext_ onto the execution context stack; moduleContext is now the running execution context.</del>
1. <del>Let _result_ be the result of evaluating _module_.[[ECMAScriptCode]].</del>
1. <del>Suspend moduleContext and remove it from the execution context stack.</del>
1. <del>Resume the context that is now on the top of the execution context stack as the running execution context.</del>
1. <del>Return Completion(_result_). </del>
1. <ins>If _module_.[[Async]] is *false*, then</ins>
1. <ins>Suspend the currently running execution context.</ins>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this isn't quite right

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants