Skip to content

Commit

Permalink
module: fix leak of vm.SyntheticModule
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
joyeecheung committed Jun 22, 2023
1 parent aa328cb commit 1751348
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 0 deletions.
1 change: 1 addition & 0 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ ModuleWrap::ModuleWrap(Environment* env,
object->SetInternalField(kURLSlot, url);
object->SetInternalField(kSyntheticEvaluationStepsSlot, undefined);
object->SetInternalField(kContextObjectSlot, undefined);
MakeWeak();
}

ModuleWrap::~ModuleWrap() {
Expand Down
24 changes: 24 additions & 0 deletions test/es-module/test-vm-synthetic-module-leak.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
'use strict';

// Flags: --experimental-vm-modules --max-heap-size=20
// This tests that vm.SyntheticModule does not leak.
// See https://github.com/nodejs/node/issues/44211

require('../common');
const vm = require('vm');

let count = 0;
async function createModule() {
const m = new vm.SyntheticModule(['bar'], () => {
m.setExport('bar', new Date().toISOString().repeat(1e6));
});
await m.link(() => {});
await m.evaluate();
count++;
if (count < 30000) {
setImmediate(createModule);
}
return m;
}

createModule();
23 changes: 23 additions & 0 deletions test/known_issues/test-vm-source-text-module-leak.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

// Flags: --experimental-vm-modules --max-heap-size=20
// This tests that using vm.compileFunction() with vm.SyntheticModule in
// the dynamic import callback does not leak.
// See https://github.com/nodejs/node/issues/44211

require('../common');
const vm = require('vm');

let count = 0;
async function createModule() {
const m = new vm.SourceTextModule('export default "hello".repeat(1e5)');
await m.link(() => {});
await m.evaluate();
count++;
if (count < 30000) {
setImmediate(createModule);
}
return m;
}

createModule();

0 comments on commit 1751348

Please sign in to comment.