Variant B promise refactoring

[guybedford]

I've made a step here towards refactoring the promise handling, based on the current suggestions of storing [[ExecPromise]] on the module record itself, as well as using the promises approach mentioned in #18 (comment).

My experience with the promises spec is very weak, so there are some gaps where I couldn't find appropriate spec functions for AwaitAll and PerformBuiltinPromiseThen, which I have left as to-dos. Help with this would be great, just let me know if I can explain them further at all.

Algorithmically the following approach is taken:

1. We need to ensure that the "InnerModuleEvaluation" still runs sync. That it doesn't right now is a spec bug. This is because the "evaluation" flag checks all assume we are doing a single depth-first-search algorithm, while once we introduce promises into the mix, overlapping executions can be running this algorithm resulting in invalid handling of stack, status and dfsIndex.
2. To make "InnerModuleEvaluation" sync, we change the meaning of "evaluated" to mean module.[[ExecPromise]] has been set. The module.[[ExecPromise]] value is the promise for full evaluation of that module, including its dependencies. We then have the exec promise either resolve or reject, which is what forms the execution completion.
3. When it comes to handling the races on evaluation here, we thus effectively have the sync algorithm setting [[ExecPromise]] for all modules in the graph. If a module already has already been executed by this specific top-level run, or another, then it will have an [[ExecPromise]] already set so we can use that.
4. For error handling, we can no longer rely on the sync population of "stack" being the exact circular subgraph of execution failure in Evaluate. Instead, we simply have the rejection of [[ExecPromise]] store this for us, allowing [[EvaluationError]] to be deprecated and avoiding having to rewrite this behaviour.
5. Each parent is effectively doing Promise.all(...depModule.[[ExecPromise]]). When an already-evaluating dependency is encountered implying a cycle, we want to avoid gridlock in this case. To do this we don't resolve the parent to its [[ExecPromise]] in this case, but to ensure error handling behaviour keeps its invariant of sharing the error throughout the failed subgraph, we do propogate its rejection.
6. Once all of that is wired up by InnerModuleEvaluation, we then simply wait on the top-level module's [[ExecPromise]] to get the completion.

@domenic I would really value your feedback re how to complete the promise handling here. I will try and give it another shot again when I can, but wanted to post this early for initial feedback.

[guybedford]

The only strange edge case I can think of with this approach is if you have a cycle like the following:

A -> B -> A

where each is awaiting, then if I run a top-level evaluation of A, and then run a top-level evaluation of B before A has resolved, then the B [[ExecPromise]] is effectively just a promise for the completion of B only, and as a result the second top-level evaluation job will return complete even if A is still executing.

This seemed to me a valid compromise for circular references, but the alternative is to make ExecPromise singular on that execution, and then separately construct a top-level evaluation promise. The issue with this approach is that we no longer store errors directly on the module records of the modules that should throw, but re-propogate up from dependencies each time when handling error caching, but this could be deemed acceptable. Reworking as discussed above can be straightforward though. Alternative ideas welcome too.

[domenic]

Just wanted to say thanks very much for this work, and it didn't go unnoticed. We're trying to marshall resources within the champion group to do proper review; both Myles and myself have gotten unfortunately busy with other projects.

In the meantime, in lieu of a full review, I'll say that the "Algorithmically the following approach is taken:" points all seem pretty sound.

For "how to complete the promise handling here", do you mean pointers on how to fill in the TODOs? The Promise.all is pretty easy and I can work on that, but for PerformBuiltinPromiseThen I wonder if you could just replace its usage with ! Call (_evalCapability_.[[Resolve]], _execution_)? There is probably a subtle reason why that is different though.

then the B [[ExecPromise]] is effectively just a promise for the completion of B only, and as a result the second top-level evaluation job will return complete even if A is still executing.

This seems pretty unfortunate. Perhaps we should work through it in more detail and discuss the alternatives in a dedicated issue? E.g. it'd be good to have concrete module bodies for A and B to evaluate what intuitively "should" happen.

[domenic]

w3ctag/promises-guide#55 has a draft of what a proper spec-level Promise.all might look like. Would need to be adapted to ECMAspeak a bit ("steps" is too wishy-washy, I guess).

[guybedford]

Just wanted to say thanks very much for this work, and it didn't go unnoticed. We're trying to marshall resources within the champion group to do proper review; both Myles and myself have gotten unfortunately busy with other projects.

I'm glad to help ensure this is as polished as possible as it reaches these stages.

For "how to complete the promise handling here", do you mean pointers on how to fill in the TODOs?

The TODOs as well as the usages of promises, ensuring the execution stack is being handled correctly.

I wonder if you could just replace its usage with ! Call (evalCapability.[[Resolve]], execution)? There is probably a subtle reason why that is different though.

Would that work if _execution_ is builtin algorithm spec steps? The important thing is to ensure that it doesn't dive into those steps when called, but that it adds those steps to the execution stack. If that is so please confirm and I can amend.

w3ctag/promises-guide#55 has a draft of what a proper spec-level Promise.all might look like. Would need to be adapted to ECMAspeak a bit ("steps" is too wishy-washy, I guess).

Actually, since we have AwaitWithReturn, and these promises have all been created already, I just did the for (const promise of promises) await promise analog in spec text which should be fine I think.

This seems pretty unfortunate. Perhaps we should work through it in more detail and discuss the alternatives in a dedicated issue? E.g. it'd be good to have concrete module bodies for A and B to evaluate what intuitively "should" happen.

Sure, this is a consequence of combining the stored execution promise with the promise of the dependencies. The alternative would be to have the execution promise only refer to the execution promise of the current module being executed, with each top-level load constructing the full promise for its tree execution (what I originally proposed here - #31 (comment) for these reasons). I can make such change certainly, just let me know.

[guybedford]

Of course the issue with for (const promise of promises) await promise is incorrect error handling. I've put together a simple implementation of awaitAll instead.

[domenic]

Would that work if execution is builtin algorithm spec steps?

Ah, I misread and thought execution was a promise for some reason. OK, this will need some sort of trickiness, nevermind.

Sure, this is a consequence of combining the stored execution promise with the promise of the dependencies. The alternative would be...

I am not really able to understand these two alternatives as stated, sorry. You are deeper into this than I am. That's why I suggested a separate issue for discussion with concrete example module bodies that we could check our understanding against.

Of course the issue with for (const promise of promises) await promise is incorrect error handling.

What's incorrect about the error handling?

I've put together a simple implementation of awaitAll instead.

FWIW your version is not technically allowed because ECMAspeak does not allow inlining of steps. I think we should keep your version, but add a note pointing to tc39/ecma262#933. Resolving that is a stage 3 -> stage 4 concern.

[guybedford]

I am not really able to understand these two alternatives as stated, sorry. You are deeper into this than I am. That's why I suggested a separate issue for discussion with concrete example module bodies that we could check our understanding against.

Ok, I was just saying I could try spec the other approach and then we can compare. The canonical example is the one stated, and the differences are not so much execution tradeoffs as just different spec approaches.

What's incorrect about the error handling?

If executing A which depends on B and C, then if there were an error in the execution of C, this would only throw after the promise for B's execution resolves. Rather we should throw failure as soon as possible for A.

FWIW your version is not technically allowed because ECMAspeak does not allow inlining of steps.

If I treated the steps as separate concrete methods would that work?

[domenic]

The canonical example is the one stated

I don't understand the one stated :). Example JavaScript code would help me understand.

the differences are not so much execution tradeoffs as just different spec approaches.

Oh, if there are no runtime differences then I guess I won't worry about this.

If executing A which depends on B and C, then if there were an error in the execution of C, this would only throw after the promise for B's execution resolves. Rather we should throw failure as soon as possible for A.

This decreases determinism, right? I.e. you'll get a different error depending on whether B or C loads faster. I think we should not throw as soon as possible in this case; we should throw only after everything completes.

If I treated the steps as separate concrete methods would that work?

Separate concrete functions? Yes, that would work; that's what most other parts of the spec do. But it decreases clarity and bloats the spec. So I think leaving it as-is is better, with a link to the issue about making it legal.

[guybedford]

I don't understand the one stated :). Example JavaScript code would help me understand.

There are runtime differences. Ok, I've created #35

(1) feels like the most sound to me, but involves a new concept of ownership. (2) is what I was suggesting specifying. and (3) is what is specified. As an extreme edge case, so I'm not too worried about which one we go with though.

This decreases determinism, right? I.e. you'll get a different error depending on whether B or C loads faster. I think we should not throw as soon as possible in this case; we should throw only after everything completes.

What I specified previously was very deterministic - the first module in order to throw is the one that throws. Perhaps we just go back to that then?

[domenic]

Thanks for opening the other issue! Will discuss there.

What I specified previously was very deterministic - the first module in order to throw is the one that throws. Perhaps we just go back to that then?

Yeah, I like that a lot.

[littledan]

At a high level, I wonder if this spec text would be written better in a style which more directly matches the existing specification, either with the Await() macro or direct usage of Promise algorithms. My biggest semantic concern is #43, though this PR doesn't change that.

[littledan]

I don't really understand the purpose of this AwaitWithReturn macro. It looks like one of the usages is attempting to get the completion record, but Await already returns a completion record. I wonder if the other usage (which involves the TBD PerformPromiseThen algorithm) would be described more clearly by specifying an algorithm which ends up calling the resolve or reject methods as appropriate.

[littledan]

I'm not sure if this is the behavior we want to have on errors. If one module's Promise rejects, shouldn't we reject this module's Promise immediately, without waiting for the earlier ones to resolve?

[littledan]

Note that the logic in w3ctag/promises-guide#55 handles failures in this way.

[littledan]

Some editorial improvements for this PR proposed at guybedford#1 . Overall, I like the semantics here, after some more consideration in #43, and some offline discussion about alternatives to deadlocking from dynamic import (where, at this point, I'm pretty convinced we shouldn't have built-in deadlock detection).

[guybedford]

This PR has now been updated with the latest spec fixes and updates here by @littledan.

[MylesBorins]

@guybedford @domenic @littledan is this ready to land in its current state? Seems like an improvement from where we are at right now and we can continue to improve in other PRs

[littledan]

I'd say it's ready to land. We will need follow-on changes, especially for #47 but also maybe for propagating errors better, but this is an improvement that those fixes can be built on.