src: clear all linked module caches once instantiated by legendecas · Pull Request #59117 · nodejs/node

legendecas · 2025-07-18T22:58:32Z

There are two phases in module linking: link, and instantiate. These
two operations are required to be separated to allow cyclic
dependencies.

v8::Module::InstantiateModule is only required to be invoked on the
root module. The global references created by ModuleWrap::Link are
only cleared at ModuleWrap::Instantiate. So the global references
created for depended modules are usually not cleared because
ModuleWrap::Instantiate is not invoked for each of depended modules,
and caused memory leak.

The change references the linked modules in an object internal slot.

This is not an issue for Node.js ESM support as these modules can not be
off-loaded. However, this could be outstanding for vm.Module.

Fixes: #50113

nodejs-github-bot · 2025-07-18T22:58:37Z

Review requested:

@nodejs/loaders
@nodejs/vm

codecov · 2025-07-18T23:57:08Z

Codecov Report

❌ Patch coverage is 73.52941% with 18 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@5e1a4fa). Learn more about missing BASE report.
⚠️ Report is 546 commits behind head on main.

Files with missing lines	Patch %	Lines
src/module_wrap.cc	74.24%	8 Missing and 9 partials ⚠️
src/module_wrap.h	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #59117   +/-   ##
=======================================
  Coverage        ?   90.05%           
=======================================
  Files           ?      648           
  Lines           ?   191089           
  Branches        ?    37449           
=======================================
  Hits            ?   172091           
  Misses          ?    11600           
  Partials        ?     7398

Files with missing lines	Coverage Δ
src/module_wrap.h	`52.94% <50.00%> (ø)`
src/module_wrap.cc	`75.13% <74.24%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

src/module_wrap.h

src/module_wrap.cc

There are two phases in module linking: link, and instantiate. These two operations are required to be separated to allow cyclic dependencies. `v8::Module::InstantiateModule` is only required to be invoked on the root module. The global references created by `ModuleWrap::Link` are only cleared at `ModuleWrap::Instantiate`. So the global references created for depended modules are usually not cleared because `ModuleWrap::Instantiate` is not invoked for each of depended modules, and caused memory leak. The change references the linked modules in an object internal slot. This is not an issue for Node.js ESM support as these modules can not be off-loaded. However, this could be outstanding for `vm.Module`.

nodejs-github-bot · 2025-07-26T23:01:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/68224/

joyeecheung

LGTM % a suggestion. Thanks!

src/module_wrap.cc

mcollina

lgtm, good spot

nodejs-github-bot · 2025-07-27T15:11:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/68241/

nodejs-github-bot · 2025-07-27T17:21:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/68242/

nodejs-github-bot · 2025-07-28T13:28:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/68269/

nodejs-github-bot · 2025-07-28T23:53:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/68278/

nodejs-github-bot · 2025-07-29T10:20:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/68295/

nodejs-github-bot · 2025-07-29T12:17:54Z

Landed in e55e0a7

joyeecheung · 2025-07-31T11:11:43Z

src/module_wrap.cc

-  Local<Object> module_object =
-      dependent->resolve_cache_[cache_key].Get(isolate);
-  if (module_object.IsEmpty() || !module_object->IsObject()) {
+  auto it = dependent->resolve_cache_.find(cache_key);


I gave this a bit of thought and I think we might be able to just save the hash/look up cost completely if we have an V8 API that just always gives us the index. Put together a prototype: https://chromium-review.googlesource.com/c/v8/v8/+/6804466

There are two phases in module linking: link, and instantiate. These two operations are required to be separated to allow cyclic dependencies. `v8::Module::InstantiateModule` is only required to be invoked on the root module. The global references created by `ModuleWrap::Link` are only cleared at `ModuleWrap::Instantiate`. So the global references created for depended modules are usually not cleared because `ModuleWrap::Instantiate` is not invoked for each of depended modules, and caused memory leak. The change references the linked modules in an object internal slot. This is not an issue for Node.js ESM support as these modules can not be off-loaded. However, this could be outstanding for `vm.Module`. PR-URL: nodejs#59117 Fixes: nodejs#50113 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Original commit message: [api] Add index-based module resolution in InstantiateModule() Add new InstantiateModule() overload that allows embedders to identify modules requests by index in the module requests array rather than using specifier and import attributes. When embedders want to fetch all the modules using information from module->GetModuleRequests() before calling InstantiateModule() instead of having to do the fetching inside the InstantiateModule() callback, they could just maintain a simple array of modules indexed by module request positions and look up the fetched the module by index in the new callback. Previously this has to be done by mapping from specifier and import attributes to module objects cached on the embedder side, leading to an overhead to hash the specifier and import attributes for each module request. Refs: nodejs#59117 Bug: 435317398 Change-Id: Ie017d2f3ccc605e0f58aa423504b5fa5fdbcc633 Refs: v8/v8@1db0e6b Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>

There are two phases in module linking: link, and instantiate. These two operations are required to be separated to allow cyclic dependencies. `v8::Module::InstantiateModule` is only required to be invoked on the root module. The global references created by `ModuleWrap::Link` are only cleared at `ModuleWrap::Instantiate`. So the global references created for depended modules are usually not cleared because `ModuleWrap::Instantiate` is not invoked for each of depended modules, and caused memory leak. The change references the linked modules in an object internal slot. This is not an issue for Node.js ESM support as these modules can not be off-loaded. However, this could be outstanding for `vm.Module`. PR-URL: #59117 Fixes: #50113 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | minor | `24.5.0` -> `24.6.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.6.0`](https://github.com/nodejs/node/releases/tag/v24.6.0): 2025-08-14, Version 24.6.0 (Current), @RafaelGSS [Compare Source](nodejs/node@v24.5.0...v24.6.0) ##### Notable Changes - \[[`471fe712b3`](nodejs/node@471fe712b3)] - **(SEMVER-MINOR)** **cli**: add NODE\_USE\_SYSTEM\_CA=1 (Joyee Cheung) [#59276](nodejs/node#59276) - \[[`38aedfbf73`](nodejs/node@38aedfbf73)] - **(SEMVER-MINOR)** **crypto**: support ML-DSA KeyObject, sign, and verify (Filip Skokan) [#59259](nodejs/node#59259) - \[[`201304537e`](nodejs/node@201304537e)] - **(SEMVER-MINOR)** **zlib**: add dictionary support to zstdCompress and zstdDecompress (lluisemper) [#59240](nodejs/node#59240) - \[[`e79c93a5d0`](nodejs/node@e79c93a5d0)] - **(SEMVER-MINOR)** **http**: add server.keepAliveTimeoutBuffer option (Haram Jeong) [#59243](nodejs/node#59243) - \[[`c144d69efc`](nodejs/node@c144d69efc)] - **lib**: docs deprecate \_http\_\* (Sebastian Beltran) [#59293](nodejs/node#59293) - \[[`aeb4de55a7`](nodejs/node@aeb4de55a7)] - **(SEMVER-MINOR)** **fs**: port SonicBoom module to fs module as Utf8Stream (James M Snell) [#58897](nodejs/node#58897) ##### Commits - \[[`f7484575ff`](nodejs/node@f7484575ff)] - **assert**: change utils to use index instead of for...of (방진혁) [#59278](nodejs/node#59278) - \[[`269cd16185`](nodejs/node@269cd16185)] - **benchmark**: remove deprecated \_extend from benchmark (Rafael Gonzaga) [#59228](nodejs/node#59228) - \[[`848e49c20b`](nodejs/node@848e49c20b)] - **benchmark**: add fs warmup to writefile-promises (Bruno Rodrigues) [#59215](nodejs/node#59215) - \[[`8c609be1b1`](nodejs/node@8c609be1b1)] - **benchmark**: add calibrate-n script (Rafael Gonzaga) [#59186](nodejs/node#59186) - \[[`6a3bf772d8`](nodejs/node@6a3bf772d8)] - **build**: fix node\_use\_sqlite for GN builds (Shelley Vohr) [#59017](nodejs/node#59017) - \[[`471fe712b3`](nodejs/node@471fe712b3)] - **(SEMVER-MINOR)** **cli**: add NODE\_USE\_SYSTEM\_CA=1 (Joyee Cheung) [#59276](nodejs/node#59276) - \[[`38aedfbf73`](nodejs/node@38aedfbf73)] - **(SEMVER-MINOR)** **crypto**: support ML-DSA KeyObject, sign, and verify (Filip Skokan) [#59259](nodejs/node#59259) - \[[`a312e706cf`](nodejs/node@a312e706cf)] - **crypto**: prepare webcrypto key import/export for modern algorithms (Filip Skokan) [#59284](nodejs/node#59284) - \[[`3a7c2c3a47`](nodejs/node@3a7c2c3a47)] - **deps**: update ada to 3.2.7 (Node.js GitHub Bot) [#59336](nodejs/node#59336) - \[[`8d9ceeaf6a`](nodejs/node@8d9ceeaf6a)] - **deps**: update archs files for openssl-3.5.2 (Node.js GitHub Bot) [#59371](nodejs/node#59371) - \[[`33b06df354`](nodejs/node@33b06df354)] - **deps**: upgrade openssl sources to openssl-3.5.2 (Node.js GitHub Bot) [#59371](nodejs/node#59371) - \[[`fa70f1af77`](nodejs/node@fa70f1af77)] - **deps**: support madvise(3C) across ALL illumos revisions (Dan McDonald) [#58237](nodejs/node#58237) - \[[`f834a6be59`](nodejs/node@f834a6be59)] - **deps**: update undici to 7.13.0 (Node.js GitHub Bot) [#59338](nodejs/node#59338) - \[[`db2417487e`](nodejs/node@db2417487e)] - **deps**: update sqlite to 3.50.4 (Node.js GitHub Bot) [#59337](nodejs/node#59337) - \[[`41978adb08`](nodejs/node@41978adb08)] - **deps**: V8: backport [`493cb53`](nodejs/node@493cb53691be) (Chengzhong Wu) [#59238](nodejs/node#59238) - \[[`05667991ca`](nodejs/node@05667991ca)] - **deps**: V8: backport [`1c3e018`](nodejs/node@1c3e018e7d48) (Renegade334) [#58818](nodejs/node#58818) - \[[`fd61588bb4`](nodejs/node@fd61588bb4)] - **doc**: rename x509.extKeyUsage to x509.keyUsage (Filip Skokan) [#59332](nodejs/node#59332) - \[[`a271ae4360`](nodejs/node@a271ae4360)] - **doc**: fix Pbkdf2Params hash attribute heading (Filip Skokan) [#59395](nodejs/node#59395) - \[[`72cfff165b`](nodejs/node@72cfff165b)] - **doc**: fix missing reference links for server.keepAliveTimeoutBuffer (Lee Jiho) [#59356](nodejs/node#59356) - \[[`8341916772`](nodejs/node@8341916772)] - **doc**: fix grammar in global dispatcher usage (Eng Zer Jun) [#59344](nodejs/node#59344) - \[[`e3e489706b`](nodejs/node@e3e489706b)] - **doc**: run license-builder (github-actions\[bot]) [#59343](nodejs/node#59343) - \[[`46527e8cea`](nodejs/node@46527e8cea)] - **doc**: correct orthography `eg.` → `e.g.` (Jacob Smith) [#59329](nodejs/node#59329) - \[[`d140c3713e`](nodejs/node@d140c3713e)] - **doc**: clarify the need of compiler compatible with c++20 (Rafael Gonzaga) [#59297](nodejs/node#59297) - \[[`95e9cabf9d`](nodejs/node@95e9cabf9d)] - **doc**: clarify release candidate stability index (Filip Skokan) [#59295](nodejs/node#59295) - \[[`a056dd36d2`](nodejs/node@a056dd36d2)] - **doc**: add WDYT to glossary (btea) [#59280](nodejs/node#59280) - \[[`1e2c52f5c4`](nodejs/node@1e2c52f5c4)] - **doc**: add manpage entry for --use-system-ca (Joyee Cheung) [#59273](nodejs/node#59273) - \[[`31a46fdeb4`](nodejs/node@31a46fdeb4)] - **doc**: add path.join and path.normalize clarification (Rafael Gonzaga) [#59262](nodejs/node#59262) - \[[`cff3725ff9`](nodejs/node@cff3725ff9)] - **doc**: fix typo in `test/common/README.md` (Yoo) [#59180](nodejs/node#59180) - \[[`31a9283591`](nodejs/node@31a9283591)] - **doc**: add note on process memoryUsage (fengmk2) [#59026](nodejs/node#59026) - \[[`5a98bff6b8`](nodejs/node@5a98bff6b8)] - **doc**: format safely for `doc-kit` (Aviv Keller) [#59229](nodejs/node#59229) - \[[`95b8b7ea5c`](nodejs/node@95b8b7ea5c)] - **domain**: remove deprecated API call (Alex Yang) [#59339](nodejs/node#59339) - \[[`2990f178bd`](nodejs/node@2990f178bd)] - **fs**: fix glob TypeError on restricted dirs (Sylphy-0xd3ac) [#58674](nodejs/node#58674) - \[[`e2fb4caf9c`](nodejs/node@e2fb4caf9c)] - **fs**: correct error message when FileHandle is transferred (Alex Yang) [#59156](nodejs/node#59156) - \[[`aeb4de55a7`](nodejs/node@aeb4de55a7)] - **(SEMVER-MINOR)** **fs**: port SonicBoom module to fs module as Utf8Stream (James M Snell) [#58897](nodejs/node#58897) - \[[`e79c93a5d0`](nodejs/node@e79c93a5d0)] - **(SEMVER-MINOR)** **http**: add server.keepAliveTimeoutBuffer option (Haram Jeong) [#59243](nodejs/node#59243) - \[[`0fb005a53f`](nodejs/node@0fb005a53f)] - **http2**: set Http2Stream#sentHeaders for raw headers (Darshan Sen) [#59244](nodejs/node#59244) - \[[`e055539604`](nodejs/node@e055539604)] - **lib**: add trace-sigint APIs (theanarkh) [#59040](nodejs/node#59040) - \[[`d2183d860a`](nodejs/node@d2183d860a)] - **lib**: optimize writable stream buffer clearing (Yoo) [#59406](nodejs/node#59406) - \[[`47543a7e17`](nodejs/node@47543a7e17)] - **lib**: handle windows reserved device names on UNC (Rafael Gonzaga) [#59286](nodejs/node#59286) - \[[`c6911f0717`](nodejs/node@c6911f0717)] - **lib**: do not modify prototype deprecated asyncResource (RafaelGSS) [#59195](nodejs/node#59195) - \[[`3c88b769bb`](nodejs/node@3c88b769bb)] - **lib**: restructure assert to become a class (Miguel Marcondes Filho) [#58253](nodejs/node#58253) - \[[`e91b54df59`](nodejs/node@e91b54df59)] - **lib**: handle superscript variants on windows device (Rafael Gonzaga) [#59261](nodejs/node#59261) - \[[`4ee467905d`](nodejs/node@4ee467905d)] - **lib**: use validateString (hotpineapple) [#59296](nodejs/node#59296) - \[[`c144d69efc`](nodejs/node@c144d69efc)] - **lib**: docs deprecate \_http\_\* (Sebastian Beltran) [#59293](nodejs/node#59293) - \[[`c89b67e681`](nodejs/node@c89b67e681)] - **lib**: add type names in source mapped stack traces (Chengzhong Wu) [#58976](nodejs/node#58976) - \[[`5b2363be8d`](nodejs/node@5b2363be8d)] - **lib**: prefer AsyncIteratorPrototype primordial (René) [#59097](nodejs/node#59097) - \[[`41b4f4d694`](nodejs/node@41b4f4d694)] - **meta**: clarify pr objection process further (James M Snell) [#59096](nodejs/node#59096) - \[[`0eb5962f1e`](nodejs/node@0eb5962f1e)] - **meta**: add mailmap entry for aditi-1400 (Aditi) [#59316](nodejs/node#59316) - \[[`a2b72c2304`](nodejs/node@a2b72c2304)] - **meta**: add tsc and build team as codeowners building.md (Rafael Gonzaga) [#59298](nodejs/node#59298) - \[[`d69f3ee1e0`](nodejs/node@d69f3ee1e0)] - **meta**: add nodejs/path to path files (Rafael Gonzaga) [#59289](nodejs/node#59289) - \[[`1e37eab865`](nodejs/node@1e37eab865)] - **node-api**: reword "implementation in an alternative VM" as implementable (Chengzhong Wu) [#59036](nodejs/node#59036) - \[[`64add6302a`](nodejs/node@64add6302a)] - **src**: use simdjson to parse SEA configuration (Joyee Cheung) [#59323](nodejs/node#59323) - \[[`e9c6636585`](nodejs/node@e9c6636585)] - **src**: mark realm leaf classes final (Anna Henningsen) [#59355](nodejs/node#59355) - \[[`42ef8147d1`](nodejs/node@42ef8147d1)] - **src**: warn about FastOneByteString invalidation (James M Snell) [#59275](nodejs/node#59275) - \[[`8686b8037a`](nodejs/node@8686b8037a)] - **src**: remove unused DSAKeyExportJob (Filip Skokan) [#59291](nodejs/node#59291) - \[[`1e5f632666`](nodejs/node@1e5f632666)] - **src**: use C++20 `contains()` method (iknoom) [#59304](nodejs/node#59304) - \[[`22d4683cfe`](nodejs/node@22d4683cfe)] - **src**: added CHECK\_NOT\_NULL check for multiple eq\_wrap\_async (F3lixTheCat) [#59267](nodejs/node#59267) - \[[`6a47ff4943`](nodejs/node@6a47ff4943)] - **src**: clear all linked module caches once instantiated (Chengzhong Wu) [#59117](nodejs/node#59117) - \[[`33728cb4ca`](nodejs/node@33728cb4ca)] - **src**: add nullptr checks in `StreamPipe::New` (Burkov Egor) [#57613](nodejs/node#57613) - \[[`4a907bdad1`](nodejs/node@4a907bdad1)] - **src**: add percentage support to --max-old-space-size (Asaf Federman) [#59082](nodejs/node#59082) - \[[`7c189d4f55`](nodejs/node@7c189d4f55)] - **test**: deflake sequential/test-tls-session-timeout (Joyee Cheung) [#59423](nodejs/node#59423) - \[[`fb0a6fb57f`](nodejs/node@fb0a6fb57f)] - **test**: exclude mock from coverage (Shima Ryuhei) [#59348](nodejs/node#59348) - \[[`7e10f95f13`](nodejs/node@7e10f95f13)] - **test**: split test-fs-cp.js (Joyee Cheung) [#59408](nodejs/node#59408) - \[[`41bcf5f659`](nodejs/node@41bcf5f659)] - **test**: update WPT resources,WebCryptoAPI,webstorage (Filip Skokan) [#59311](nodejs/node#59311) - \[[`f9f3dc94cb`](nodejs/node@f9f3dc94cb)] - **test**: add known issue test for fs.cpSync dereference bug (James M Snell) [#58941](nodejs/node#58941) - \[[`244d0c38a8`](nodejs/node@244d0c38a8)] - **test**: deflake stream-readable-to-web test (Ethan Arrowood) [#58948](nodejs/node#58948) - \[[`564e604a1a`](nodejs/node@564e604a1a)] - **test**: make test-inspector-network-resource sequential (Shima Ryuhei) [#59104](nodejs/node#59104) - \[[`7ab13b7477`](nodejs/node@7ab13b7477)] - **test**: don't use expose internals in test-http-outgoing-buffer.js (Meghan Denny) [#59219](nodejs/node#59219) - \[[`319df3859a`](nodejs/node@319df3859a)] - **test,crypto**: skip unsupported ciphers (Shelley Vohr) [#59388](nodejs/node#59388) - \[[`713c70c32a`](nodejs/node@713c70c32a)] - **test\_runner**: remove unused callee convertion (Alex Yang) [#59221](nodejs/node#59221) - \[[`e4ca30e115`](nodejs/node@e4ca30e115)] - **tools**: disable nullability-completeness warnings (Michaël Zasso) [#59392](nodejs/node#59392) - \[[`dab7f6b542`](nodejs/node@dab7f6b542)] - **tools**: check for std::vector\<v8::Local> in lint (Aditi) [#58497](nodejs/node#58497) - \[[`7b94982eb0`](nodejs/node@7b94982eb0)] - **tools**: allow selecting test subsystems with numbers in their names (Darshan Sen) [#59242](nodejs/node#59242) - \[[`16bbcd8881`](nodejs/node@16bbcd8881)] - **typings**: improve internal binding types (Nam Yooseong) [#59351](nodejs/node#59351) - \[[`76bc4d659b`](nodejs/node@76bc4d659b)] - **typings**: improve internal binding types (Michaël Zasso) [#59176](nodejs/node#59176) - \[[`eecd3272a6`](nodejs/node@eecd3272a6)] - **worker**: add name for worker (theanarkh) [#59213](nodejs/node#59213) - \[[`84c3513ce2`](nodejs/node@84c3513ce2)] - **worker**: implements nits in Web Locks code (Antoine du Hamel) [#59270](nodejs/node#59270) - \[[`bd68fbd753`](nodejs/node@bd68fbd753)] - **worker**: add cpuUsage for worker (theanarkh) [#59177](nodejs/node#59177) - \[[`201304537e`](nodejs/node@201304537e)] - **(SEMVER-MINOR)** **zlib**: add dictionary support to zstdCompress and zstdDecompress (lluisemper) [#59240](nodejs/node#59240) </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. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Original commit message: [api] Add index-based module resolution in InstantiateModule() Add new InstantiateModule() overload that allows embedders to identify modules requests by index in the module requests array rather than using specifier and import attributes. When embedders want to fetch all the modules using information from module->GetModuleRequests() before calling InstantiateModule() instead of having to do the fetching inside the InstantiateModule() callback, they could just maintain a simple array of modules indexed by module request positions and look up the fetched the module by index in the new callback. Previously this has to be done by mapping from specifier and import attributes to module objects cached on the embedder side, leading to an overhead to hash the specifier and import attributes for each module request. Refs: nodejs#59117 Bug: 435317398 Change-Id: Ie017d2f3ccc605e0f58aa423504b5fa5fdbcc633 Refs: v8/v8@1db0e6b Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>

Original commit message: [api] Add index-based module resolution in InstantiateModule() Add new InstantiateModule() overload that allows embedders to identify modules requests by index in the module requests array rather than using specifier and import attributes. When embedders want to fetch all the modules using information from module->GetModuleRequests() before calling InstantiateModule() instead of having to do the fetching inside the InstantiateModule() callback, they could just maintain a simple array of modules indexed by module request positions and look up the fetched the module by index in the new callback. Previously this has to be done by mapping from specifier and import attributes to module objects cached on the embedder side, leading to an overhead to hash the specifier and import attributes for each module request. Refs: nodejs#59117 Bug: 435317398 Change-Id: Ie017d2f3ccc605e0f58aa423504b5fa5fdbcc633 Refs: v8/v8@46d7bc7 Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>

richardlau · 2025-09-19T17:08:23Z

This doesn't land cleanly on v22.x-staging so a manual backport will be necessary if this is to be released there.

There are two phases in module linking: link, and instantiate. These two operations are required to be separated to allow cyclic dependencies. `v8::Module::InstantiateModule` is only required to be invoked on the root module. The global references created by `ModuleWrap::Link` are only cleared at `ModuleWrap::Instantiate`. So the global references created for depended modules are usually not cleared because `ModuleWrap::Instantiate` is not invoked for each of depended modules, and caused memory leak. The change references the linked modules in an object internal slot. This is not an issue for Node.js ESM support as these modules can not be off-loaded. However, this could be outstanding for `vm.Module`. PR-URL: nodejs#59117 Fixes: nodejs#50113 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

There are two phases in module linking: link, and instantiate. These two operations are required to be separated to allow cyclic dependencies. `v8::Module::InstantiateModule` is only required to be invoked on the root module. The global references created by `ModuleWrap::Link` are only cleared at `ModuleWrap::Instantiate`. So the global references created for depended modules are usually not cleared because `ModuleWrap::Instantiate` is not invoked for each of depended modules, and caused memory leak. The change references the linked modules in an object internal slot. This is not an issue for Node.js ESM support as these modules can not be off-loaded. However, this could be outstanding for `vm.Module`. PR-URL: #59117 Backport-PR-URL: #60152 Fixes: #50113 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jul 18, 2025

legendecas added module Issues and PRs related to the module subsystem. vm Issues and PRs related to the vm subsystem. labels Jul 18, 2025

legendecas force-pushed the module-instantiate branch from af391e1 to 352631b Compare July 18, 2025 23:06

addaleax reviewed Jul 21, 2025

View reviewed changes

src/module_wrap.h Outdated Show resolved Hide resolved

src/module_wrap.cc Outdated Show resolved Hide resolved

joyeecheung reviewed Jul 21, 2025

View reviewed changes

src/module_wrap.cc Outdated Show resolved Hide resolved

legendecas force-pushed the module-instantiate branch from 352631b to eb17cb7 Compare July 21, 2025 23:46

joyeecheung reviewed Jul 22, 2025

View reviewed changes

src/module_wrap.cc Outdated Show resolved Hide resolved

joyeecheung reviewed Jul 22, 2025

View reviewed changes

src/module_wrap.cc Outdated Show resolved Hide resolved

legendecas force-pushed the module-instantiate branch 2 times, most recently from 30ab1d2 to afd12e1 Compare July 24, 2025 22:46

legendecas force-pushed the module-instantiate branch from afd12e1 to 7886911 Compare July 26, 2025 22:55

joyeecheung approved these changes Jul 27, 2025

View reviewed changes

src/module_wrap.cc Outdated Show resolved Hide resolved

src/module_wrap.cc Outdated Show resolved Hide resolved

mcollina approved these changes Jul 27, 2025

View reviewed changes

fixup! src: clear all linked module caches once instantiated

0313c1f

joyeecheung approved these changes Jul 29, 2025

View reviewed changes

legendecas added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 29, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 29, 2025

nodejs-github-bot merged commit e55e0a7 into nodejs:main Jul 29, 2025
60 checks passed

legendecas deleted the module-instantiate branch July 29, 2025 12:18

joyeecheung reviewed Jul 31, 2025

View reviewed changes

legendecas mentioned this pull request Aug 5, 2025

vm: sync-ify SourceTextModule linkage #59000

Merged

github-actions bot mentioned this pull request Aug 11, 2025

2025-08-12, Version 24.6.0 (Current) #59449

Merged

legendecas mentioned this pull request Aug 15, 2025

Jest unit tests failing after upgrading to 24.6.X #59480

Closed

prabhu mentioned this pull request Aug 17, 2025

chore(deps): update node.js to v24.6.0 cdxgen/cdxgen#2177

Merged

1 task

busma13 mentioned this pull request Aug 18, 2025

Node 24.6 module linking changes break jest tests terascope/teraslice#4141

Closed

richardlau added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Sep 19, 2025

legendecas mentioned this pull request Oct 7, 2025

[v22.x] vm: sync-ify SourceTextModule linkage #60152

Closed

legendecas added backport-open-v22.x Indicate that the PR has an open backport and removed backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Oct 7, 2025

aduh95 added backported-to-v22.x PRs backported to the v22.x-staging branch. and removed backport-open-v22.x Indicate that the PR has an open backport labels Oct 11, 2025

github-actions bot mentioned this pull request Oct 12, 2025

2025-10-20, Version 22.21.0 'Jod' (LTS) #60230

Merged

joyeecheung mentioned this pull request Jan 29, 2026

vm.SourceTextModule memory leak #59118

Closed

Uh oh!

Conversation

legendecas commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 18, 2025

Uh oh!

codecov bot commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 26, 2025

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 27, 2025

Uh oh!

nodejs-github-bot commented Jul 27, 2025

Uh oh!

nodejs-github-bot commented Jul 28, 2025

Uh oh!

nodejs-github-bot commented Jul 28, 2025

Uh oh!

nodejs-github-bot commented Jul 29, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 29, 2025

Uh oh!

joyeecheung Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardlau commented Sep 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

legendecas commented Jul 18, 2025 •

edited

Loading

codecov bot commented Jul 18, 2025 •

edited

Loading

joyeecheung Jul 31, 2025 •

edited

Loading