FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

vm: "afterEvaluate", evaluate() return a promise from the outer context · nodejs/node@312b33a · GitHub

/ node Public

Commit 312b33a

Browse files
ericrannaud
authored and
aduh95
committed
vm: "afterEvaluate", evaluate() return a promise from the outer context
Consider the default context A with a microtask queue QA, and a context B with its own microtask queue QB. Context B is constructed with vm.createContext(..., {microtaskMode: "afterEvaluate"}). The evaluation in context B can be performed via vm.Script or vm.SourceTextModule. The standard (https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob) dictates that, when resolving a {promise} with {resolution}, from any context, the {then} method on {promise} should be called within a task enqueued on the microtask queue from the context associated with {then}. Specifically, after evaluating a script or module in context B, any promises created within B, if later resolved within A, will result in a task to be enqueued back onto QB, even long after we are done evaluating any code within B. This creates a challenge for users of node:vm in "afterEvaluate" mode. In ContextifyScript::EvalMachine() and in ModuleWrap::Evaluate(), we only drain the microtask queue QB a single time after running the script or evaluating the module. After that point, the queue will not be drained unless another script or module is evaluated in the same context. In the following scenario, prior to this patch, the log statement will not be printed: const microtaskMode = "afterEvaluate"; const context = vm.createContext({}, {microtaskMode}); const source = ""; const module = new vm.SourceTextModule(source, {context}); await module.link(() => null); await module.evaluate(); console.log("NOT PRINTED"); Within `evaluate()`, there is this `await` statement: await this[kWrap].evaluate(timeout, breakOnSigint) Since the promise returned by ModuleWrap::Evaluate() is the top-level capability for {module}, a promise created within B, V8 will enqueue a task on QB. But since this is after the PerformCheckpoint() call in ModuleWrap::Evaluate(), the task in QB is never run. In the meantime, since QA is empty, the Node process simply exits (with a warning about the unsettled promise, if it happened to be a top-level await). While being unable to do `await module.evaluate()` is clearly a problem, more generally, it is intended that in "afterEvaluate" mode, promises created in the inner context cannot make progress if, and until, the microtask queue of the inner context is checkpointed. Therefore, to address this issue, the fix is narrow: When the module has its own microtask queue, i.e. in "afterEvaluate" mode, the inner-context promise returned by v8::SourceTextModule::Evaluate() is first resolved to an outer-context promise, then we checkpoint the microtask queue of the inner context, then we return the outer-context promise we just built. This ensures that in the statement `await this[kWrap].evaluate(...)`, the promise returned can be resolved within the outer context, without involving the microtask queue in the inner context. Fixes: #59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ PR-URL: #59801 Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent ca1942c commit 312b33a

2 files changed

Lines changed: 45 additions & 6 deletions

File tree

‎src/module_wrap.cc‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -630,8 +630,48 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
630630
MaybeLocal<Value> result;
631631
auto run = [&]() {
632632
MaybeLocal<Value> result = module->Evaluate(context);
633-
if (!result.IsEmpty() && microtask_queue)
633+
634+
Local<Value> res;
635+
if (result.ToLocal(&res) && microtask_queue) {
636+
DCHECK(res->IsPromise());
637+
638+
// To address https://github.com/nodejs/node/issues/59541 when the
639+
// module has its own separate microtask queue in microtaskMode
640+
// "afterEvaluate", we avoid returning a promise built inside the
641+
// module's own context.
642+
//
643+
// Instead, we build a promise in the outer context, which we resolve
644+
// with {result}, then we checkpoint the module's own queue, and finally
645+
// we return the outer-context promise.
646+
//
647+
// If we simply returned the inner promise {result} directly, per
648+
// https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob, the outer
649+
// context, when resolving a promise coming from a different context,
650+
// would need to enqueue a task (known as a thenable job task) onto the
651+
// queue of that different context (the module's context). But this queue
652+
// will normally not be checkpointed after evaluate() returns.
653+
//
654+
// This means that the execution flow in the outer context would
655+
// silently fall through at the statement (in lib/internal/vm/module.js):
656+
// await this[kWrap].evaluate(timeout, breakOnSigint)
657+
//
658+
// This is true for any promises created inside the module's context
659+
// and made available to the outer context, as the node:vm doc explains.
660+
//
661+
// We must handle this particular return value differently to make it
662+
// possible to await on the result of evaluate().
663+
Local<Context> outer_context = isolate->GetCurrentContext();
664+
Local<Promise::Resolver> resolver;
665+
if (!Promise::Resolver::New(outer_context).ToLocal(&resolver)) {
666+
return MaybeLocal<Value>();
667+
}
668+
if (resolver->Resolve(outer_context, res).IsNothing()) {
669+
return MaybeLocal<Value>();
670+
}
671+
result = resolver->GetPromise();
672+
634673
microtask_queue->PerformCheckpoint(isolate);
674+
}
635675
return result;
636676
};
637677
if (break_on_sigint && timeout != -1) {

‎test/parallel/test-vm-module-after-evaluate.js‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ const microtaskMode = 'afterEvaluate';
1414

1515
(async () => {
1616
const mustNotCall1 = common.mustNotCall();
17-
const mustNotCall2 = common.mustNotCall();
17+
const mustCall1 = common.mustCall();
1818

1919
const inner = {};
2020

@@ -28,7 +28,6 @@ const microtaskMode = 'afterEvaluate';
2828
await module.link(mustNotCall1);
2929
await module.evaluate();
3030

31-
// This is Issue 59541, the next statement is not executed, of course
32-
// it should be.
33-
mustNotCall2();
34-
})().then(common.mustNotCall());
31+
// Prior to the fix for Issue 59541, the next statement was never executed.
32+
mustCall1();
33+
})().then(common.mustCall());

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL