Abrupt completion in InnerModuleEvaluation always calls HostPromiseRejectionTracker
syg opened this issue · 14 comments
The spec seems to mandate that whenever InnerModuleEvaluation returns an abrupt completion, HostPromiseRejectionTracker must be called.
Evaluate step 6 creates the top level Promise capability, but this capability does not escape to script until Evaluate() returns. That is, TopLevelModuleEvaluationJob doesn't have a chance to hook up the rejection handler until the promise is already rejected. So, in step 9.d in the case of errors, calling [[Reject]] on the capability will always end up calling HostPromiseRejectionTracker by way of RejectPromise step 7.
I consider this a spec bug. The top-level capability in Evaluate is an internal one used to report async module evaluation errors to the host via HostReportErrors. It should not also go through HostPromiseRejectionTracker.
The most straightforward fix is probably splitting up Evaluate() into a PrepareForEvaluate() that returns the top-level capability and the rest of Evaluate().
@MylesBorins Would node be in favor of this change?
What's the observable effect of this change?
What's the observable effect of this change?
For context, this question was raised in v8:9970, there's a test case in there.
The observable effect of the proposed change is that modules that throw will no longer unconditionally call HostPromiseRejectionTracker. This, of course, depends on whatever observable things HostPromiseRejectionTracker does. If the host uses that to report unhandled Promise rejections, the observable effect would be you stop getting spurious unhandled rejection messages when a module errors.
HostPromiseRejectionHandler has a case for this: When a handler is added to a rejected promise for the first time, it is called with its operation argument set to "handle".
The problem with V8 is that the promise is rejected and Evaluate() also throws (returns an abrupt completion) which is not how the spec works.
The unhandled rejection we faced in node was because we do if (try_catch.HasCaught()) { try_catch.ReThrow(); return; } and lost the rejected promise.
The problem with V8 is that the promise is rejected and Evaluate() also throws (returns an abrupt completion) which is not how the spec works.
Indeed, the throwing part will be fixed.
This still seems like a spec bug to me. There is no good reason that the rejection handler for the top-level capability is always added after the Promise is already rejected that I can tell. It doesn't escape to script.
Would node be in favor of this change?
I suppose we'd save a few milliseconds on a graph not by dealing with rejection handler calls, which would be nice, but we attach a catch handler within the same tick, so it isn't a huge problem.
FWIW, doing Promise.reject() (or closer to what the spec is doing, new Promise((r, j) => j()) also guarantees that HostPromiseRejectionTracker is called.
Maybe we could do a module-namespace-style memoized GetModuleCapability(module) so hosts can use it if they want, but it isn't needed in an evaluation process.
FWIW, doing Promise.reject() (or closer to what the spec is doing, new Promise((r, j) => j()) also guarantees that HostPromiseRejectionTracker is called.
I understand for a user-exposed Promise that unconditionally rejects in the executor, there is no way to attach a handler before rejection. The intent of the top-level capability isn't an unconditionally rejected promise. Moreover, the handler is also unconditionally added, just at the wrong time.
Maybe we could do a module-namespace-style memoized GetModuleCapability(module) so hosts can use it if they want, but it isn't needed in an evaluation process.
Are you arguing against changing Evaluate? What is the value in calling a hook that is designed for unhandled rejections for a rejection that is always handled, and in calling two host error reporting hooks for every module evaluation error?
I'm just thinking the common case would be
capability = module.PrepareForEvaluate()
module.Evaluate()
return capability
in which case you'd still have the same problem. You'd have to explicitly introduce one tick of indirection on a catch handler like this:
promise = NewPromise()
capability = module.PrepareForEvaluate()
// wait a tick before rejecting the outer-most promise
capability
.then(() => Resolve(promise))
.catch(error => Reject(promise, error))
module.Evaluate()
return promise
In node.js at least, adding another promise around it all would probably add more of a cost, and it would also hurt the ideal fast path of none of your modules failing.
Perhaps I'm not grokking the Promise semantics. Isn't a rejection job enqueued in the same way in both cases? The current spec results in something like:
- Let promise be the new promise from evaluate.
- If module evaluation throws error, reject promise. Call HostPromiseRejectionTracker.
- TopLevelModuleEvaluationJob calls promise.catch(() => HostReportErrors()). Calls HostPromiseRejectionTracker again and enqueues a new rejection job. (That is, the catch handler is synchronously called.)
I'm saying at whatever your outer-most boundary is (import(), TopLevelModuleEvaluationJob, etc.), you have to introduce a time between acquiring the promise and the promise rejecting. It's basically the zalgo problem. My point here was that this does two potentially unsavory things: it introduces an extra tick in both the resolve and reject paths, and it requires more calls between c++ and js to set up the extra promises. Whether that's better or worse than the promise rejection tracker calls is not something I can answer without testing, but I thought it was worth pointing out.
I don't think this is observable within HTML, which re-checks whether [[PromiseIsHandled]] is true from a queued task before reporting an error to users. I'd sort of hope that other environments use similar behavior, otherwise they'll get lots of other kinds of false positives (e.g., every single time you have an expression like Promise.reject().catch()).
From the dynamic import case in the issue, this in fact is a rejected Promise, and it should lead to the Promise rejection tracker, if the .catch() isn't present. It shouldn't throw a synchronous exception, just reject the Promise.
From the import statement case, the HTML spec does the equivalent of a .catch() from spec text in this algorithm using the "upon rejection" wording. Thanks to @Ms2ger , you can now find the definition in WebIDL.
As an aside, TopLevelModuleEvaluationJob is not reached from HTML, and I'm proposing to remove it in tc39/ecma262#1597
The way the specs organize this is by deferring to the embedder to list and cancel these. HTML contains an "about to be notified rejected promises list" (gotta love those variable names!) to queue these before later re-checking them, from a queued task, which gives that .catch() time to run.
I suppose the intermediate queue logic could be moved down into the ES spec if we want, and/or the V8 API could decide to be higher-level with respect to this logic, but it's a little difficult for me to figure out how that would work, since it's all tied together with when HTML gets around to actually trigger the events.
cc @domenic for Promise rejection tracker logic
Thanks @littledan for the detailed answer.
As an aside, TopLevelModuleEvaluationJob is not reached from HTML, and I'm proposing to remove it in tc39/ecma262#1597
If the editor group (my own fault here!) had better prioritized this perhaps this confusion wouldn't have arisen as much. I was all going off the assumption this was not dead code.
In light of Dan's answer, I will close this issue and prioritize with the editor group to get tc39/ecma262#1597 merged.