-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Normative: Fix hangs with top-level await #3357
base: main
Are you sure you want to change the base?
Conversation
Closes tc39#3356. Modules that depend on modules with top-level await but do not themselves have a top-level await may currently hang. When a module with TLA finishes evaluating, it triggers evaluation of ancestors modules that depend on it. Currently, ancestors that do not have TLA themselves are evaluated and have their [[Status]] set to ~evaluated~ but incorrectly leaves their [[AsyncEvaluation]] field unchanged as true. This means subsequent importers of those ancestors consider them as in the middle of async evaluation and will wait on them in InnerModuleEvaluation. But since they are already evaluated, those waits cause a hang. This PR sets [[AsyncEvaluation]] to false for those ancestors when their [[Status]] transition to ~evaluated~. Note that the ancestors that error out during evaluation do not need this fix because there is a bail-out path for errored out modules in InnerModuleEvaluation.
Thanks for finding this bug. Agreed it's a spec bug. In terms of the fix, my concern is that we would need to consistently ensure in the state model for all possible success and failure cases that An alternative fix that doesn't require this level of vetting might be to just add an extra check to the conditional in Perhaps something like this in 1. Let _requiredModule_ be GetImportedModule(_module_, _required_).
1. Set _index_ to ? InnerModuleEvaluation(_requiredModule_, _stack_, _index_).
1. If _requiredModule_ is a Cyclic Module Record, then
1. Assert: _requiredModule_.[[Status]] is one of ~evaluating~, ~evaluating-async~, or ~evaluated~.
1. Assert: _requiredModule_.[[Status]] is ~evaluating~ if and only if _stack_ contains _requiredModule_.
1. If _requiredModule_.[[Status]] is ~evaluating~, then
1. Set _module_.[[DFSAncestorIndex]] to min(_module_.[[DFSAncestorIndex]], _requiredModule_.[[DFSAncestorIndex]]).
1. Else,
1. Set _requiredModule_ to _requiredModule_.[[CycleRoot]].
1. Assert: _requiredModule_.[[Status]] is either ~evaluating-async~ or ~evaluated~.
- 1. If _requiredModule_.[[EvaluationError]] is not ~empty~, return ? _requiredModule_.[[EvaluationError]].
+ 1. If _requiredModule_.[[Status]] is ~evaluated~, then
+ 1. If _requiredModule_.[[EvaluationError]] is not ~empty~, return ? _requiredModule_.[[EvaluationError]].
- 1. If _requiredModule_.[[AsyncEvaluation]] is *true*, then
+ 1. Else if _requiredModule_.[[AsyncEvaluation]] is *true*, then
1. Set _module_.[[PendingAsyncDependencies]] to _module_.[[PendingAsyncDependencies]] + 1.
1. Append _module_ to _requiredModule_.[[AsyncParentModules]]. |
My preference would be to use [[AsyncEvaluation]] only for ordering, and use [[Status]] for state machine transitions. In an offline convo with @lucacasonato he said maybe that doesn't quite work but I didn't dig into it further. @guybedford WDYT? |
At the same time, I'll argue against myself in #3357 (comment). I want to resist a large refactoring which is likely to introduce more latent undiscovered bugs, and do the most incremental thing here. |
@syg if I'm understanding you right, you're referring to a bigger refactoring? To dig into the discussion then (and please backtrack if I'm misunderstanding) the other feature of To fully refactor |
I... think I see. If [[Status]] is unreliable as part of the same cycle we're currently iterating in, is that an issue for your preferred alternative fix in #3357 (comment)? |
Great. I would love to refactor to make
I believe it does not, since the change I've suggested there would still check both conditions (that is, if the dependency has already been processed in the current Tarjan cycle processing, it's not something we need to "listen" to, the important case, and case not captured by the I'd need to dig into this further, but I wonder if we could do a check of either the module being in evaluating or the module being in stack but before the level of stack where dfsindex equals dfsancestor as a replacement of the AsyncEvaluation check... |
Yep, I think doing the most incremental fix we can in this PR and leave the refactoring for a follow-up is the most prudent course of action. |
Okay, I can aim to look into a follow-up after this lands. For this PR for now, the fix looks good, although for a consistent model let's also set it to |
Recapping editor call discussion: will set it to false in the rejection closure for consistency with a note that the value is not consulted in the error path. |
Closes #3356.
Modules that depend on modules with top-level await but do not themselves have a top-level await may currently hang. When a module with TLA finishes evaluating, it triggers evaluation of ancestors modules that depend on it.
Currently, ancestors that do not have TLA themselves are evaluated and have their [[Status]] set to
evaluatedbut incorrectly leaves their [[AsyncEvaluation]] field unchanged as true. This means subsequent importers of those ancestors consider them as in the middle of async evaluation and will wait on them in InnerModuleEvaluation. But since they are already evaluated, those waits cause a hang.This PR sets [[AsyncEvaluation]] to false for those ancestors when their [[Status]] transition to
evaluated.Note that the ancestors that error out during evaluation do not need this fix because there is a bail-out path for errored out modules in InnerModuleEvaluation.