[v22.x] vm: sync-ify SourceTextModule linkage#60152
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
Collaborator
nodejs-github-bot commented Oct 7, 2025
Review requested:
|
nodejs-github-bot added c++ 
legendecas added vm Collaborator
nodejs-github-bot commented Oct 7, 2025
MemberAuthor
legendecas commented Oct 8, 2025
It seems like the build failure on v22.x-staging branch is caused by #59960 |
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#59117Fixes: nodejs#50113 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
This allows overriding linked requests for a `ModuleWrap`. The `statusOverride` in `vm.SourceTextModule` could call `moduleWrap.link` a second time when `statusOverride` of `linking` is set to undefined. Overriding of linked requests should be no harm but better to be avoided. However, this will require a follow-up fix on `statusOverride` in `vm.SourceTextModule`. PR-URL: nodejs#59527Fixes: nodejs#59480 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
Split `module.link(linker)` into two synchronous step `sourceTextModule.linkRequests()` and `sourceTextModule.instantiate()`. This allows creating vm modules and resolving the dependencies in a complete synchronous procedure. This also makes `syntheticModule.link()` redundant. The link step for a SyntheticModule is no-op and is already taken care in the constructor by initializing the binding slots with the given export names. PR-URL: nodejs#59000 Refs: nodejs#37648 Reviewed-By: Joyee Cheung <[email protected]>
aduh95force-pushed the backport-59000-to-22 branch from 6cbc783 to 59855d0CompareCollaborator
nodejs-github-bot commented Oct 11, 2025
aduh95 approved these changes Oct 11, 2025
aduh95 pushed a commit that referenced this pull request Oct 11, 2025
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: #60152Fixes: #50113 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
aduh95 pushed a commit that referenced this pull request Oct 11, 2025
This allows overriding linked requests for a `ModuleWrap`. The `statusOverride` in `vm.SourceTextModule` could call `moduleWrap.link` a second time when `statusOverride` of `linking` is set to undefined. Overriding of linked requests should be no harm but better to be avoided. However, this will require a follow-up fix on `statusOverride` in `vm.SourceTextModule`. PR-URL: #59527 Backport-PR-URL: #60152Fixes: #59480 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
aduh95 pushed a commit that referenced this pull request Oct 11, 2025
Split `module.link(linker)` into two synchronous step `sourceTextModule.linkRequests()` and `sourceTextModule.instantiate()`. This allows creating vm modules and resolving the dependencies in a complete synchronous procedure. This also makes `syntheticModule.link()` redundant. The link step for a SyntheticModule is no-op and is already taken care in the constructor by initializing the binding slots with the given export names. PR-URL: #59000 Backport-PR-URL: #60152 Refs: #37648 Reviewed-By: Joyee Cheung <[email protected]>
Contributor
aduh95 commented Oct 11, 2025
Landed in ef005d0...6277058 |
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
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.
src: clear all linked module caches once instantiated
There are two phases in module linking: link, and instantiate. These
two operations are required to be separated to allow cyclic
dependencies.
v8::Module::InstantiateModuleis only required to be invoked on theroot module. The global references created by
ModuleWrap::Linkareonly cleared at
ModuleWrap::Instantiate. So the global referencescreated for depended modules are usually not cleared because
ModuleWrap::Instantiateis 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 [email protected]
Reviewed-By: Matteo Collina [email protected]
module: allow overriding linked requests for a ModuleWrap
This allows overriding linked requests for a
ModuleWrap. ThestatusOverrideinvm.SourceTextModulecould callmoduleWrap.linka second time when
statusOverrideoflinkingis set to undefined.Overriding of linked requests should be no harm but better to be
avoided. However, this will require a follow-up fix on
statusOverridein
vm.SourceTextModule.PR-URL: #59527
Fixes: #59480
Reviewed-By: Joyee Cheung [email protected]
Reviewed-By: Marco Ippolito [email protected]
vm: sync-ify SourceTextModule linkage
Split
module.link(linker)into two synchronous stepsourceTextModule.linkRequests()andsourceTextModule.instantiate(). This allows creating vm modules andresolving the dependencies in a complete synchronous procedure.
This also makes
syntheticModule.link()redundant. The link step for aSyntheticModule is no-op and is already taken care in the constructor
by initializing the binding slots with the given export names.
PR-URL: #59000
Refs: #37648
Reviewed-By: Joyee Cheung [email protected]