Normative: Parent completion ordering DFS invariant extension

[guybedford]

This extension provides an approach to the case brought up in #158 where parent completion orderings entirely differing from the normal post-order execution might be seen as surprising behaviour.

This approach uses a new global execution ordering on async execution.

The async fulfillment is then split out into two separate phases: a first phase to gather the list of parent modules that are synchronously ready for execution at async fulfillment time of a dependent, and a second phase to execute this list in global execution-sorted order based on this ordering.

Async rejections are handled as previously.

This change only affects execution timings of deferred async executions, imposing this new post-ordering sort on them. If we consider module graphs where every module depends individually on a single module using top-level await, using the same well-defined post-order execution ordering on all sync parent subgraph execution completions that run together seems like a sensible adjustment to me to ensure predictable behaviour.

Fixes #158.

Further review / feedback very welcome. // cc @Ms2ger @littledan @MylesBorins @robpalme @sokra

[Ms2ger]

Yulia: I think you've got more of the proposal paged in than me right now, so I'd appreciate it if you could take a look. Let me know if you prefer I try.

[guybedford]

I've rebased this to the PR bug fix in #159, and also separated out the cycle gathering on failure, as cycle errors are allowed to be sparse in the current module algorithm, it doesn't seem such an issue to ensure consistency by just relying on cycle root. Instead we can follow this up in a separate PR if necessary.

Thanks for feedback so far. The diff should now be a little simpler as well.

[guybedford]

@jorendorff I've included your suggestions - brings the diff down nicely as well.

[guybedford]

I've rebased this PR to the latest merge of the cycle root fix, should be much simpler to review. Further feedback welcome, I do think this is an important normative change to make. If it needs to go explicitly through the meeting that would be understandable.

Further feedback would be very welcome to understand implementer interest.

[codehag]

I have unfortunately run out of time this week to fully do this review. If it can wait a week, I will do it then -- or someone can also take over. I made a few small comments.

I want to still make sure that we don't inadvertently end up changing this behaviour: https://github.com/web-platform-tests/wpt/pull/26771/files

Guy says it shouldn't be an issue but I would like to take more time to verify the implications.

[codehag]

Still thinking about the order of evaluation. A couple of comments so far

[codehag]

Bar the issue I found with three tests, that appears to be a js shell issue (will verify that today), this looks fine and is probably a worthwhile change. It doesn't appear to have web platform implications so we are good there. I am setting this to approved.

Sketch implementation here: https://phabricator.services.mozilla.com/D105437

[guybedford]

@syg it would be great to get your review on this PR if you're able to take a look.

[guybedford]

Just to reiterate, this PR is only to ensure that all sync subgraphs that depend on async modules execute in the original ordering, and it doesn't affect any of the other execution properties of this proposal that have already been agreed.

[syg]

I started implementing this and ran into a concern. I'd like the postorder position to be scoped to each top-level call to Evaluate(). However, this doesn't seem possible because the same async module can participate in concurrent, multiple top-level calls to Evaluate(), e.g. multiple <script> tags. That presents a problem: the postorder would need to be maintained as a global serial number or something, which puts it at higher risk of overflowing. There can be cleverer designs, I suppose, of resetting the global serial to 0 after all outstanding modules are finished, but that's too much complexity for my liking.

Edit: Perhaps I was premature on this concern: since the modules are always guaranteed to execute in global ascending order of when they first had their [[AsyncEvaluating]] set to true, in the global serial number approach, once a module finishes evaluating such that its ordinal is >= the global serial number, the global can be reset to 0. I think that works, but probably should draw up a proof to be safe.

[syg]

Another issue: what actually resolves the top level Promise?

[guybedford]

@syg nicely spotted - yes with the flattening of the previous recursive algorithm we need to now explicitly call the capabilities on the sync module parents of async modules.

Further to your suggestion, the fix can possibly actually resolve capabilities on all sync parents of async modules, as they could all have eg a later dynamic import expression that attached to those modules specifically. For this reason I've added the capability resolution within the main exec loop to all sync modules.

I also went ahead and moved the main "top level parent" capability resolution to the top, this is to ensure that the current module promise will resolve before the sync parent promises. Since all capability resolutions, I believe, wiill go into the task queue, the resolution functions can't be called synchronously. So having the capability resolution at the top should not break the sync invariant between the parent executions by calling a resolve function earlier than the parent executions. Please verify that I'm correct in that assumption though, and just let me know if further clarification would help.

Will also await on @codehag when she gets back for review before merging so we have time here too.

[syg]

Further to your suggestion, the fix can possibly actually resolve capabilities on all sync parents of async modules, as they could all have eg a later dynamic import expression that attached to those modules specifically.

Great point!

[syg]

If someone wants to an executable implementation in V8 to check things with, here's the CL: https://chromium-review.googlesource.com/c/v8/v8/+/2757901

[sokra]

If somebody knows how to build a node.js version from that, you could run https://github.com/evanw/tla-fuzzer with that change and check the result. That's how I found the original case.

[codehag]

Looks good to me.

[guybedford]

I tested the patch in Node.js and can confirm it fixes #158 and has the correct timings for dynamic import registrations with the latest commit as well.

[jugglinmike]

We've merged a test for the new semantics to Test262: tc39/test262#2989