-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
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
vm: "afterEvaluate", make module.evaluate() return a promise from the outer context #59801
Conversation
Review requested:
|
9f3b583
to
9cb3575
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59801 +/- ##
==========================================
+ Coverage 88.26% 88.42% +0.15%
==========================================
Files 701 703 +2
Lines 206640 207411 +771
Branches 39740 40002 +262
==========================================
+ Hits 182388 183397 +1009
+ Misses 16279 15988 -291
- Partials 7973 8026 +53
🚀 New features to boost your workflow:
|
9cb3575
to
abf9d31
Compare
@addaleax @legendecas Please have a look at the updated PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hands down the best PR I've seen all week. Thank you for taking care of this so diligently! 💙
I've added two suggestions, but feel free to consider them optional.
@ericrannaud Just to be sure – if you moved this PR to draft because of the CI failures, those do not seem to be related to this PR (except for the commit message one – we typically mark test-only commits as |
Thanks @addaleax! I have just found something odd with this change. I will report back. |
Check that we lose the execution flow in the outer context, upon resolving a promise created in in the inner context.
@addaleax False alert. I was suddenly unable to demonstrate the issue using the testcase at the first commit... A series of bad builds made me question life, the universe, and everything. I have no explanation for it, but starting from a clean build directory restored my sanity. I was trying to check whether I updated the explanation in the commit message and code comment. I also added the following note to the docs:
|
abf9d31
to
ea42008
Compare
ea42008
to
a4334e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericrannaud Okay, that makes complete sense – again, big thanks to you for being so careful here! 💙
Commit Queue failed- Loading data for nodejs/node/pull/59801 ✔ Done loading data for nodejs/node/pull/59801 ----------------------------------- PR info ------------------------------------ Title vm: "afterEvaluate", make module.evaluate() return a promise from the outer context (#59801) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch ericrannaud:vm-after-evaluate-link-queues -> nodejs:main Labels c++, vm, lib / src, author ready, needs-ci, commit-queue-rebase Commits 3 - test: testcase demonstrating issue 59541 - vm: "afterEvaluate", make module.evaluate() return a promise from the… - vm: document challenge of sharing promises between contexts w/ afterE… Committers 1 - Eric Rannaud <[email protected]> PR-URL: https://github.com/nodejs/node/pull/59801 Fixes: https://github.com/nodejs/node/issues/59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/59801 Fixes: https://github.com/nodejs/node/issues/59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 08 Sep 2025 05:15:23 GMT ✔ Approvals: 1 ✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/59801#pullrequestreview-3247483205 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-09-20T00:20:29Z: https://ci.nodejs.org/job/node-test-pull-request/69288/ - Querying data for job/node-test-pull-request/69288/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 59801 From https://github.com/nodejs/node * branch refs/pull/59801/merge -> FETCH_HEAD ✔ Fetched commits as 4612c793cb90..a4334e35ff4d -------------------------------------------------------------------------------- [main a2f413c504] test: testcase demonstrating issue 59541 Author: Eric Rannaud <[email protected]> Date: Sun Sep 7 16:09:42 2025 -0700 1 file changed, 34 insertions(+) create mode 100644 test/parallel/test-vm-module-after-evaluate.js Auto-merging src/module_wrap.cc [main 7b2bd50a53] vm: "afterEvaluate", make module.evaluate() return a promise from the outer context Author: Eric Rannaud <[email protected]> Date: Wed Sep 10 10:51:53 2025 -0700 2 files changed, 45 insertions(+), 6 deletions(-) Auto-merging doc/api/vm.md [main c72644ebd2] vm: document challenge of sharing promises between contexts w/ afterEvaluate Author: Eric Rannaud <[email protected]> Date: Fri Sep 19 07:47:31 2025 -0700 3 files changed, 167 insertions(+) create mode 100644 test/parallel/test-vm-script-after-evaluate.js ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- test: testcase demonstrating issue 59541https://github.com/nodejs/node/actions/runs/17880297406 |
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: nodejs#59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ
a4334e3
to
d70b25c
Compare
@addaleax I shortened the two offending commit subject lines. I pushed over, I hope that was the right thing to do here. |
@ericrannaud Sure, although things like that can also be addressed while merging manually, it's just that it prevents the bot from doing so. But this should be all good to go now, just requires a new CI run because of the re-push 👍 |
Landed in aa6838c...c81b1df |
Check that we lose the execution flow in the outer context, upon resolving a promise created in in the inner context. PR-URL: #59801 Fixes: #59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
PR-URL: #59801 Fixes: #59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]>
Check that we lose the execution flow in the outer context, upon resolving a promise created in in the inner context. PR-URL: #59801 Fixes: #59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
PR-URL: #59801 Fixes: #59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]>
Check that we lose the execution flow in the outer context, upon resolving a promise created in in the inner context. PR-URL: #59801 Fixes: #59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
PR-URL: #59801 Fixes: #59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]>
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | minor | `24.8.0` -> `24.9.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>nodejs/node (node)</summary> ### [`v24.9.0`](https://github.com/nodejs/node/releases/tag/v24.9.0): 2025-09-25, Version 24.9.0 (Current), @​targos [Compare Source](nodejs/node@v24.8.0...v24.9.0) ##### Notable Changes - \[[`9b043a9096`](nodejs/node@9b043a9096)] - **(SEMVER-MINOR)** **http**: add shouldUpgradeCallback to let servers control HTTP upgrades (Tim Perry) [#​59824](nodejs/node#59824) - \[[`a6456ab90a`](nodejs/node@a6456ab90a)] - **(SEMVER-MINOR)** **sqlite**: cleanup ERM support and export Session class (James M Snell) [#​58378](nodejs/node#58378) - \[[`5563361d22`](nodejs/node@5563361d22)] - **(SEMVER-MINOR)** **sqlite**: add tagged template (0hm☘️) [#​58748](nodejs/node#58748) - \[[`04013ee933`](nodejs/node@04013ee933)] - **(SEMVER-MINOR)** **worker**: add heap profile API (theanarkh) [#​59846](nodejs/node#59846) ##### Commits - \[[`cbec4fd6de`](nodejs/node@cbec4fd6de)] - **benchmark**: calibrate config dgram multi-buffer (Bruno Rodrigues) [#​59696](nodejs/node#59696) - \[[`9a4bbdc3c5`](nodejs/node@9a4bbdc3c5)] - **benchmark**: calibrate config cluster/echo.js (Nam Yooseong) [#​59836](nodejs/node#59836) - \[[`0b284d86e8`](nodejs/node@0b284d86e8)] - **build**: add the missing macro definitions for OpenHarmony (hqzing) [#​59804](nodejs/node#59804) - \[[`43e6e54d66`](nodejs/node@43e6e54d66)] - **build**: do not include custom ESLint rules testing in tarball (Antoine du Hamel) [#​59809](nodejs/node#59809) - \[[`039ac19154`](nodejs/node@039ac19154)] - **crypto**: expose signatureAlgorithm on X509Certificate (Patrick Costa) [#​59235](nodejs/node#59235) - \[[`647c332704`](nodejs/node@647c332704)] - **crypto**: use `return await` when returning Promises from async functions (Renegade334) [#​59841](nodejs/node#59841) - \[[`8ed4587cf0`](nodejs/node@8ed4587cf0)] - **crypto**: use async functions for non-stub Promise-returning functions (Renegade334) [#​59841](nodejs/node#59841) - \[[`bb051c56ef`](nodejs/node@bb051c56ef)] - **crypto**: avoid calls to `promise.catch()` (Renegade334) [#​59841](nodejs/node#59841) - \[[`05e560dd25`](nodejs/node@05e560dd25)] - **deps**: update googletest to [`50b8600`](nodejs/node@50b8600) (Node.js GitHub Bot) [#​59955](nodejs/node#59955) - \[[`fa40d3a785`](nodejs/node@fa40d3a785)] - **deps**: update archs files for openssl-3.5.3 (Node.js GitHub Bot) [#​59901](nodejs/node#59901) - \[[`8c85570d18`](nodejs/node@8c85570d18)] - **deps**: upgrade openssl sources to openssl-3.5.3 (Node.js GitHub Bot) [#​59901](nodejs/node#59901) - \[[`b71125664e`](nodejs/node@b71125664e)] - **deps**: update undici to 7.16.0 (Node.js GitHub Bot) [#​59830](nodejs/node#59830) - \[[`dea5dd7077`](nodejs/node@dea5dd7077)] - **dgram**: restore buffer optimization in fixBufferList (Yoo) [#​59934](nodejs/node#59934) - \[[`b0c1e67532`](nodejs/node@b0c1e67532)] - **diagnostics\_channel**: fix race condition with diagnostics\_channel and GC (Ugaitz Urien) [#​59910](nodejs/node#59910) - \[[`0b37b594c3`](nodejs/node@0b37b594c3)] - **doc**: use "WebAssembly" instead of "Web Assembly" (Tobias Nießen) [#​59954](nodejs/node#59954) - \[[`1e723f9c6b`](nodejs/node@1e723f9c6b)] - **doc**: fix typo in section on microtask order (Tobias Nießen) [#​59932](nodejs/node#59932) - \[[`a28962a85c`](nodejs/node@a28962a85c)] - **doc**: update V8 fast API guidance (René) [#​58999](nodejs/node#58999) - \[[`bd767c5d1b`](nodejs/node@bd767c5d1b)] - **doc**: add security escalation policy (Ulises Gascón) [#​59806](nodejs/node#59806) - \[[`9df91e59e1`](nodejs/node@9df91e59e1)] - **doc**: type improvement of file `http.md` (yusheng chen) [#​58189](nodejs/node#58189) - \[[`e4f571680b`](nodejs/node@e4f571680b)] - **doc**: deprecate closing `fs.Dir` on garbage collection (Livia Medeiros) [#​59839](nodejs/node#59839) - \[[`e9cb986fa5`](nodejs/node@e9cb986fa5)] - **doc**: rephrase dynamic import() description (Nam Yooseong) [#​59224](nodejs/node#59224) - \[[`026d4e33f7`](nodejs/node@026d4e33f7)] - **doc,crypto**: update subtle.generateKey and subtle.importKey (Filip Skokan) [#​59851](nodejs/node#59851) - \[[`2b2591db52`](nodejs/node@2b2591db52)] - **esm**: make hasAsyncGraph non-enumerable (Joyee Cheung) [#​59905](nodejs/node#59905) - \[[`993f05d323`](nodejs/node@993f05d323)] - **fs,win**: do not add a second trailing slash in readdir (Gerhard Stöbich) [#​59847](nodejs/node#59847) - \[[`7aec53b607`](nodejs/node@7aec53b607)] - **(SEMVER-MINOR)** **http**: add shouldUpgradeCallback to let servers control HTTP upgrades (Tim Perry) [#​59824](nodejs/node#59824) - \[[`83ae6102e7`](nodejs/node@83ae6102e7)] - **http**: optimize checkIsHttpToken for short strings (방진혁) [#​59832](nodejs/node#59832) - \[[`6695067636`](nodejs/node@6695067636)] - **http,https**: handle IPv6 with proxies (Joyee Cheung) [#​59894](nodejs/node#59894) - \[[`c5d910a0a9`](nodejs/node@c5d910a0a9)] - **http2**: fix allowHttp1+Upgrade, broken by shouldUpgradeCallback (Tim Perry) [#​59924](nodejs/node#59924) - \[[`acada1fb82`](nodejs/node@acada1fb82)] - **inspector**: ensure adequate memory allocation for `Binary::toBase64` (René) [#​59870](nodejs/node#59870) - \[[`396cc8ec65`](nodejs/node@396cc8ec65)] - **lib**: update inspect output format for subclasses (Miguel Marcondes Filho) [#​59687](nodejs/node#59687) - \[[`fed1dac8de`](nodejs/node@fed1dac8de)] - **lib**: update isDeepStrictEqual to support options (Miguel Marcondes Filho) [#​59762](nodejs/node#59762) - \[[`d785929fd7`](nodejs/node@d785929fd7)] - **lib**: add source map support for assert messages (Chengzhong Wu) [#​59751](nodejs/node#59751) - \[[`ff13d1d61e`](nodejs/node@ff13d1d61e)] - **lib,src**: cache ModuleWrap.hasAsyncGraph (Chengzhong Wu) [#​59703](nodejs/node#59703) - \[[`b200cd8470`](nodejs/node@b200cd8470)] - **lib,src**: refactor assert to load error source from memory (Chengzhong Wu) [#​59751](nodejs/node#59751) - \[[`e94c57301b`](nodejs/node@e94c57301b)] - **meta**: add .npmrc with ignore-scripts=true (Joyee Cheung) [#​59914](nodejs/node#59914) - \[[`728472a57b`](nodejs/node@728472a57b)] - **module**: only put directly require-d ESM into require.cache (Joyee Cheung) [#​59874](nodejs/node#59874) - \[[`be48760b93`](nodejs/node@be48760b93)] - **node-api**: added SharedArrayBuffer api (Mert Can Altin) [#​59071](nodejs/node#59071) - \[[`f006a14522`](nodejs/node@f006a14522)] - **node-api**: make napi\_delete\_reference use node\_api\_basic\_env (Jeetu Suthar) [#​59684](nodejs/node#59684) - \[[`0f46c1c3b0`](nodejs/node@0f46c1c3b0)] - **repl**: fix cpu overhead pasting big strings to the REPL (Ruben Bridgewater) [#​59857](nodejs/node#59857) - \[[`3eeb7b47ea`](nodejs/node@3eeb7b47ea)] - **sqlite**: fix crash session extension callbacks with workers (Bart Louwers) [#​59848](nodejs/node#59848) - \[[`0fe53375ec`](nodejs/node@0fe53375ec)] - **(SEMVER-MINOR)** **sqlite**: cleanup ERM support and export Session class (James M Snell) [#​58378](nodejs/node#58378) - \[[`9a3e58a007`](nodejs/node@9a3e58a007)] - **(SEMVER-MINOR)** **sqlite**: add tagged template (0hm☘️) [#​58748](nodejs/node#58748) - \[[`f14ed5ab7b`](nodejs/node@f14ed5ab7b)] - **src**: simplify watchdog instantiations via `std::optional` (Anna Henningsen) [#​59960](nodejs/node#59960) - \[[`e330f03f84`](nodejs/node@e330f03f84)] - **src**: update crypto objects to use DictionaryTemplate (James M Snell) [#​59942](nodejs/node#59942) - \[[`69b5607cf4`](nodejs/node@69b5607cf4)] - **src**: simplify is\_callable by making it a concept (Tobias Nießen) [#​58169](nodejs/node#58169) - \[[`86150f3401`](nodejs/node@86150f3401)] - **src**: rename private fields to follow naming convention (Moonki Choi) [#​59923](nodejs/node#59923) - \[[`d17f299539`](nodejs/node@d17f299539)] - **src**: use DictionaryTemplate more in URLPattern (James M Snell) [#​59892](nodejs/node#59892) - \[[`ac784912ac`](nodejs/node@ac784912ac)] - **src**: reduce the nearest parent package JSON cache size (Michael Smith) [#​59888](nodejs/node#59888) - \[[`abecdcb536`](nodejs/node@abecdcb536)] - **src**: replace FIXED\_ONE\_BYTE\_STRING with Environment-cached strings (Moonki Choi) [#​59891](nodejs/node#59891) - \[[`2bb152500b`](nodejs/node@2bb152500b)] - **src**: create strings in `FIXED_ONE_BYTE_STRING` as internalized (Anna Henningsen) [#​59826](nodejs/node#59826) - \[[`03116a7cd8`](nodejs/node@03116a7cd8)] - **src**: remove `std::array` overload of `FIXED_ONE_BYTE_STRING` (Anna Henningsen) [#​59826](nodejs/node#59826) - \[[`8a5325d6e3`](nodejs/node@8a5325d6e3)] - **src**: ensure `v8::Eternal` is empty before setting it (Anna Henningsen) [#​59825](nodejs/node#59825) - \[[`f0c20ccd81`](nodejs/node@f0c20ccd81)] - **src**: remove unnecessary `Environment::GetCurrent()` calls (Moonki Choi) [#​59814](nodejs/node#59814) - \[[`213188e491`](nodejs/node@213188e491)] - **stream**: use new AsyncResource instead of bind (Matteo Collina) [#​59867](nodejs/node#59867) - \[[`ce8435b003`](nodejs/node@ce8435b003)] - **test**: testcase demonstrating issue 59541 (Eric Rannaud) [#​59801](nodejs/node#59801) - \[[`8f32746142`](nodejs/node@8f32746142)] - **test**: guard write to proxy client if proxy connection is ended (Joyee Cheung) [#​59742](nodejs/node#59742) - \[[`6790093fcb`](nodejs/node@6790093fcb)] - **tls**: load bundled and extra certificates off-thread (Joyee Cheung) [#​59856](nodejs/node#59856) - \[[`f5d3f919d8`](nodejs/node@f5d3f919d8)] - **tls**: only do off-thread certificate loading on loading tls (Joyee Cheung) [#​59856](nodejs/node#59856) - \[[`87bbaa23a0`](nodejs/node@87bbaa23a0)] - **tools**: fix `tools/make-v8.sh` for clang (Richard Lau) [#​59893](nodejs/node#59893) - \[[`0d23fd525b`](nodejs/node@0d23fd525b)] - **tools**: skip test-internet workflow for draft MRs (Michaël Zasso) [#​59817](nodejs/node#59817) - \[[`e17c73731a`](nodejs/node@e17c73731a)] - **tools**: copyedit `build-tarball.yml` (Antoine du Hamel) [#​59808](nodejs/node#59808) - \[[`97c4e1bac9`](nodejs/node@97c4e1bac9)] - **typings**: remove unused imports (Nam Yooseong) [#​59880](nodejs/node#59880) - \[[`8b29bbca76`](nodejs/node@8b29bbca76)] - **url**: replaced slice with at (Mikhail) [#​59181](nodejs/node#59181) - \[[`6458867a6b`](nodejs/node@6458867a6b)] - **url**: add type checking to urlToHttpOptions() (simon-id) [#​59753](nodejs/node#59753) - \[[`3c62b3886f`](nodejs/node@3c62b3886f)] - **util**: inspect objects with throwing Symbol.toStringTag (Ruben Bridgewater) [#​59860](nodejs/node#59860) - \[[`6133a82875`](nodejs/node@6133a82875)] - **util**: fix debuglog.enabled not being present with callback logger (Ruben Bridgewater) [#​59858](nodejs/node#59858) - \[[`9347ddddf4`](nodejs/node@9347ddddf4)] - **vm**: explain how to share promises between contexts w/ afterEvaluate (Eric Rannaud) [#​59801](nodejs/node#59801) - \[[`44ce971619`](nodejs/node@44ce971619)] - **vm**: "afterEvaluate", evaluate() return a promise from the outer context (Eric Rannaud) [#​59801](nodejs/node#59801) - \[[`6e586a1409`](nodejs/node@6e586a1409)] - **vm**: expose hasTopLevelAwait on SourceTextModule (Chengzhong Wu) [#​59865](nodejs/node#59865) - \[[`49747a58a3`](nodejs/node@49747a58a3)] - **(SEMVER-MINOR)** **worker**: add heap profile API (theanarkh) [#​59846](nodejs/node#59846) - \[[`b970c0bbc2`](nodejs/node@b970c0bbc2)] - **zlib**: reduce code duplication (jhofstee) [#​57810](nodejs/node#57810) - \[[`9782ca2b1b`](nodejs/node@9782ca2b1b)] - **zlib**: implement fast path for crc32 (Gürgün Dayıoğlu) [#​59813](nodejs/node#59813) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS4xMzAuMCIsInVwZGF0ZWRJblZlciI6IjQxLjEzMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Check that we lose the execution flow in the outer context, upon resolving a promise created in in the inner context. PR-URL: #59801 Fixes: #59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]>
PR-URL: #59801 Fixes: #59541 Refs: https://issues.chromium.org/issues/441679231 Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob Reviewed-By: Anna Henningsen <[email protected]>
The underlying behavior conforms to the spec, see Refs. We need a workaround on the Node side to at least enable the use of
await
on the return value ofmodule.evaluate()
.This series first adds a test that confirm the current incorrect behavior: I wrote the test so it passes with the current code.
Then I introduce a workaround (credit for the idea goes to @addaleax) and update the tests to verify that the behavior is now correct.
Then I add a subsection to
doc/vm.md
to explain how it can be a challenge to share promises between contexts with afterEvaluate; I also add more tests to demonstrate how promises behave in this case.Fixes: #59541
Refs: https://issues.chromium.org/issues/441679231
Refs: https://groups.google.com/g/v8-dev/c/YIeRg8CUNS8/m/rEQdFuNZAAAJ
Refs: https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob