-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
vm: "afterEvaluate", make module.evaluate() return a promise from the outer context #59801
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
Closed
ericrannaud
wants to merge
3
commits into
nodejs:main
from
ericrannaud:vm-after-evaluate-link-queues
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
// Flags: --experimental-vm-modules | ||
'use strict'; | ||
|
||
// https://github.com/nodejs/node/issues/59541 | ||
// | ||
// Promises created in a context using microtaskMode: "aferEvaluate" (meaning it | ||
// has its own microtask queue), when resolved in the surrounding context, will | ||
// schedule a task back onto the inner context queue. | ||
|
||
const common = require('../common'); | ||
const vm = require('vm'); | ||
|
||
const microtaskMode = 'afterEvaluate'; | ||
|
||
(async () => { | ||
const mustNotCall1 = common.mustNotCall(); | ||
const mustNotCall2 = common.mustNotCall(); | ||
const mustCall1 = common.mustCall(); | ||
|
||
const inner = {}; | ||
|
||
const context = vm.createContext({ inner }, { microtaskMode }); | ||
|
||
const module = new vm.SourceTextModule( | ||
'inner.promise = Promise.resolve();', | ||
{ context }, | ||
); | ||
|
||
await module.link(mustNotCall1); | ||
await module.evaluate(); | ||
|
||
// Prior to the fix for Issue 59541, the next statement was never executed. | ||
mustCall1(); | ||
|
||
await inner.promise; | ||
|
||
// This is expected: the await statement above enqueues a (thenable job) task | ||
// onto the inner context microtask queue, but it will not be checkpointed, | ||
// therefore we never make progress. | ||
mustNotCall2(); | ||
})().then(common.mustNotCall()); | ||
|
||
(async () => { | ||
const mustNotCall1 = common.mustNotCall(); | ||
const mustCall1 = common.mustCall(); | ||
const mustCall2 = common.mustCall(); | ||
const mustCall3 = common.mustCall(); | ||
|
||
const inner = {}; | ||
|
||
const context = vm.createContext({ inner }, { microtaskMode }); | ||
|
||
const module = new vm.SourceTextModule( | ||
'inner.promise = Promise.resolve();', | ||
{ context }, | ||
); | ||
|
||
await module.link(mustNotCall1); | ||
await module.evaluate(); | ||
ericrannaud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
mustCall1(); | ||
|
||
setImmediate(() => { | ||
mustCall2(); | ||
// This will checkpoint the inner context microtask queue, and allow the | ||
// promise from the inner context to be resolved in the outer context. | ||
module.evaluate(); | ||
}); | ||
|
||
await inner.promise; | ||
mustCall3(); | ||
})().then(common.mustCall()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
'use strict'; | ||
|
||
// https://github.com/nodejs/node/issues/59541 | ||
// | ||
// Promises created in a context using microtaskMode: "aferEvaluate" (meaning | ||
// it has its own microtask queue), when resolved in the surrounding context, | ||
// will schedule a task back onto the inner context queue. This test checks that | ||
// the async execution progresses normally. | ||
|
||
const common = require('../common'); | ||
const vm = require('vm'); | ||
|
||
const microtaskMode = 'afterEvaluate'; | ||
|
||
(async () => { | ||
const mustNotCall1 = common.mustNotCall(); | ||
|
||
await vm.runInNewContext( | ||
`Promise.resolve()`, | ||
{}, { microtaskMode }); | ||
|
||
// Expected behavior: resolving an promise created in the inner context, from | ||
// the outer context results in the execution flow falling through, unless the | ||
// inner context microtask queue is manually drained, which we don't do here. | ||
mustNotCall1(); | ||
})().then(common.mustNotCall()); | ||
|
||
(async () => { | ||
const mustCall1 = common.mustCall(); | ||
const mustCall2 = common.mustCall(); | ||
const mustCall3 = common.mustCall(); | ||
|
||
// Create a new context. | ||
const context = vm.createContext({}, { microtaskMode }); | ||
|
||
setImmediate(() => { | ||
// This will drain the context microtask queue, after the `await` statement | ||
// below, and allow the promise from the inner context, created below, to be | ||
// resolved in the outer context. | ||
vm.runInContext('', context); | ||
mustCall2(); | ||
}); | ||
|
||
const inner_promise = vm.runInContext( | ||
`Promise.resolve()`, | ||
context); | ||
mustCall1(); | ||
|
||
await inner_promise; | ||
mustCall3(); | ||
})().then(common.mustCall()); | ||
|
||
{ | ||
const mustNotCall1 = common.mustNotCall(); | ||
const mustCall1 = common.mustCall(); | ||
|
||
const context = vm.createContext({ setImmediate, mustNotCall1 }, { microtaskMode }); | ||
|
||
// setImmediate() will be run after runInContext() returns, and since the | ||
// anonymous function passed to `then` is defined in the inner context, the | ||
// thenable job task will be enqueued on the inner context microtask queue, | ||
// but at this point, it will not be drained automatically. | ||
vm.runInContext(`new Promise(setImmediate).then(() => mustNotCall1())`, context); | ||
|
||
mustCall1(); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.