GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — module: rework of memory management in `vm` APIs with the `importModuleDynamically` option by joyeecheung · Pull Request #48510 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Jun 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This PR (hopefully) fixes a bunch of memory leaks & use-after-free surrounding vm.Script, vm.compileFunction, vm.SyntheticModule and vm.SourceTextModule when dynamic import is used (see the issues referenced below) and makes it possible for people to upgrade from older versions of Node.js.

This cannot land as-is on v20.x as it depends on #49491 and #49419 which break the ABI on v20x. We need a special Node.js v20-only extension to the V8 API for this to work without breaking the ABI.

module: use symbol in WeakMap to manage host defined options

Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

module: fix leak of vm.SyntheticModule

Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

module: fix the leak in SourceTextModule and ContextifySript

Replace the persistent handles to v8::Module and
v8::UnboundScript with an internal reference that V8's GC is
aware of to fix the leaks.

Refs: #44211
Refs: #42080
Refs: #47096
Refs: #43205
Refs: #38695

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

adanski, rezonant, sirDonovan, viceice, bricss, shrujalshah28, kirillgroshkov, tonivj5, JP250552, hartherbert, and 24 more reacted with thumbs up emoji viceice, bricss, sapphi-red, kirillgroshkov, tonivj5, jakebailey, MindFreeze, ln-e, jimmy-guzman, oskarols, and 9 more reacted with heart emoji wyardley, Xunnamius, and dnalborczyk reacted with rocket emoji shrujalshah28, bricss, kirillgroshkov, tonivj5, ln-e, oskarols, EmilyRosina, andriyor, brunocabral88, blimmer, and 2 more reacted with eyes emoji
All reactions
  • 👍 34 reactions
  • ❤️ 19 reactions
  • 🚀 3 reactions
  • 👀 12 reactions
@nodejs-github-bot

nodejs-github-bot commented Jun 21, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/vm
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 21, 2023
@joyeecheung
joyeecheung marked this pull request as draft June 21, 2023 16:01
@joyeecheung joyeecheung changed the title WIP: fix vm.compileFunction() dynamic import callback leak WIP: fix leak in vm.compileFunction() dynamic import callback and the ModuleWrap Jun 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 21, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jun 21, 2023
vm: fix leak in vm.compileFunction when importModuleDynamically is used #46785
Closed
vm: memory leak on vm.compileFunction with importModuleDynamically #44211
Closed
@joyeecheung

joyeecheung commented Jun 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Still need to do a bit more investigation to figure out it's really safe this time around, so marking it as WIP (I've done some walkthrough of the vm.compileFunction() part, but haven't really looked into the ModuleWrap part). I suppose we probably also want to put it behind flags until symbol-as-weakmap-key gets to stage 4 though. Also need to think of more test cases to exercise the new memory management.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95
aduh95 reviewed Jun 21, 2023
View reviewed changes
Comment thread lib/internal/vm/module.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/vm/module.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/create_dynamic_module.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/modules/esm/translators.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

ljharb
ljharb reviewed Jun 21, 2023
View reviewed changes
Comment thread lib/internal/vm.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/vm.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented Jun 22, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Looks like this fixes the segfault in #43205 and #38695 as well

adanski, kirillgroshkov, JP250552, lin72h, and Qard reacted with hooray emoji tonivj5, targos, kirillgroshkov, lin72h, and Qard reacted with rocket emoji
All reactions
  • 🎉 5 reactions
  • 🚀 5 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung joyeecheung mentioned this pull request Jun 22, 2023
Segmentation fault when executing worker with eval: true containing dynamic import after await #43205
Closed
@joyeecheung joyeecheung changed the title WIP: fix leak in vm.compileFunction() dynamic import callback and the ModuleWrap WIP: use symbol in weakmap to maintain lifetime of modules' host defined options Jun 22, 2023
@targos

targos commented Jun 22, 2023

Copy link
Copy Markdown
Member

amazing!

JakobJingleheimer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hildjj

hildjj commented Jun 22, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

It looks to me like symbols-as-weakmap-keys is now Stage 4 as of 2023-01-30, but not all of the documents have been updated.

I would guess back-porting this fix to Node 16 or 18 is going to be a challenge though, right?

kirillgroshkov and lin72h reacted with thumbs up emoji adanski and vidal7 reacted with eyes emoji
All reactions
  • 👍 2 reactions
  • 👀 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung
joyeecheung force-pushed the fix-compile branch 2 times, most recently from 1751348 to 76f5ce9 Compare June 22, 2023 16:47
@joyeecheung

joyeecheung commented Jun 22, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

It looks to me like symbols-as-weakmap-keys is now Stage 4 as of 2023-01-30, but not all of the documents have been updated.

Nice. Then I think we don't have to flag this for v20. But yeah v18 or v16 didn't implement that proposal (and we probably shouldn't backport it / cannot do a full upgrade because LTS) so it cannot be backported.

I've tweaked the registry object a bit, added tests, and some analysis to the commit messages about why it fixes the leaks/segfaults. Although I think for the vm.Modules this just fixes vm.SyntheticModule, but vm.SourceTextModule would still leak until we find a way to create ModuleWrap -> v8::Module references without using the problematic v8::Persistent approach (maybe using the namespace object to transitively hold a reference to the Module would work, but that feels hacky to me, so I'll leave it for now).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@joyeecheung
joyeecheung force-pushed the fix-compile branch from 76f5ce9 to 9b07d0c Compare June 22, 2023 16:56
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@joyeecheung
joyeecheung force-pushed the fix-compile branch from 9b07d0c to 3fca427 Compare June 22, 2023 17:18
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jun 23, 2023
CI Reliability 2023-06-23 nodejs/reliability#596
Open
CI Reliability 2023-06-24 nodejs/reliability#597
Open
CI Reliability 2023-06-25 nodejs/reliability#598
Open
@sutt0n sutt0n mentioned this pull request Jun 26, 2023
[Bug]: Memory consumption issues on Node JS 16.11.0+ jestjs/jest#11956
Closed
@joyeecheung
joyeecheung force-pushed the fix-compile branch from aace036 to 59a6ca5 Compare June 27, 2023 18:46
This was referenced Oct 5, 2023
SourceTextModule memory leak #33439
Closed
chore: replace vm.Script with vm.compileFunction jestjs/jest#12205
Closed
This was referenced Oct 23, 2023
await Promise.resolve("import('./mod.mjs')").then(eval) rejects/segfaults in REPL #43681
Closed
module: migrate to script context based host defined options #44923
Closed
SIGSEGV Segmentation fault when loading data URIs using import #44438
Closed
@joberstein joberstein mentioned this pull request Oct 29, 2023
fix: update dependency cosmiconfig to v8.3.6 - autoclosed conventional-changelog/commitlint#3654
Closed
1 task
@SimenB SimenB mentioned this pull request Oct 30, 2023
[Bug]: False positive memory leak when running with --experimental-vm-modules and using --detectLeaks jestjs/jest#14649
Closed
This was referenced Nov 30, 2023
VM's context when evaluating ESM script not garbage-collected (works with CJS) #50964
Closed
[18.x] vm: backport vm-related fixes #51004
Closed
@davidmehren davidmehren mentioned this pull request Jan 18, 2024
replace flexsearch-ts with orama and upgrade to next 14 hedgedoc/hedgedoc#5354
Merged
3 tasks done
@hildjj

hildjj commented Jan 28, 2024

Copy link
Copy Markdown

@joyeecheung Is there a way to detect this fix from unprivileged code that is better than checking a bunch of version numbers?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@codebytere codebytere mentioned this pull request Mar 1, 2024
fix: node::loader::ImportModuleDynamically crash electron/electron#41491
Merged
4 tasks done
@alexvy86 alexvy86 mentioned this pull request Mar 8, 2024
fix(ci): Disable jest cache for app-insights-logger tests microsoft/FluidFramework#20037
Merged
@robhogan robhogan mentioned this pull request Mar 13, 2024
Support ESM react-native.config files react-native-community/cli#2167
Closed
@richardlau richardlau mentioned this pull request Mar 20, 2024
v18.20.0 proposal #52165
Merged
@joyeecheung joyeecheung mentioned this pull request May 22, 2024
cli: add --expose-gc flag available to NODE_OPTIONS #53078
Merged
@eyalroth eyalroth mentioned this pull request Jul 12, 2025
[Bug]: --detectLeaks CPU hammering on Node 20 jestjs/jest#15706
Closed
@joyeecheung joyeecheung mentioned this pull request Oct 16, 2025
module: refactor and clarify async loader hook customizations #60278
Merged
@joyeecheung joyeecheung mentioned this pull request Jan 29, 2026
SourceTextModule: Using dynamic imports while passing cachedData throws TypeError: Invalid host defined options #43663
Closed
@SimenB SimenB mentioned this pull request May 8, 2026
vm: SourceTextModule memory leak - instances retained via async context frames after evaluate() #63186
Open
@vishalranaut vishalranaut mentioned this pull request Jun 14, 2026
vm: fix SourceTextModule memory leak #63913
Closed
This was referenced Aug 4, 2026
无法在不重启的前提下重载部件 steve02081504/fount#11
Open
Server-side single-module reload: what is the plan after #34944 was closed? denoland/deno#36408
Closed
@robobun robobun mentioned this pull request Sep 13, 2026
node:vm: break Strong<> cycle through NodeVMScriptFetcher importModuleDynamically callback oven-sh/bun#29866
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
@Qard Qard Qard approved these changes
@benjamingr benjamingr benjamingr approved these changes
@aduh95 aduh95 aduh95 approved these changes
+1 more reviewer
@TMaszko TMaszko TMaszko left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@joyeecheung @nodejs-github-bot @targos @hildjj @SimenB @schmod @brunocabral88 @legendecas @aduh95 @ljharb @Qard @benjamingr @TMaszko
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.