[v22.x] vm: sync-ify SourceTextModule linkage - #60152
Closed
legendecas wants to merge 3 commits into
Closed
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
|
Review requested:
|
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
It seems like the build failure on v22.x-staging branch is caused by #59960 |
Sorry, something went wrong.
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>
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#59527 Fixes: nodejs#59480 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
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 <joyeec9h3@gmail.com>
aduh95
force-pushed
the
backport-59000-to-22
branch
from
6cbc783 to
59855d0
Compare
October 11, 2025 18:51
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
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: #60152 Fixes: #50113 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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: #60152 Fixes: #59480 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
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 <joyeec9h3@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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::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
module: allow overriding linked requests for a ModuleWrap
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
Fixes: #59480
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Marco Ippolito marcoippolito54@gmail.com
vm: sync-ify SourceTextModule linkage
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
Refs: #37648
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com