-
Notifications
You must be signed in to change notification settings - Fork 30k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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.
- Loading branch information
1 parent
f27339a
commit 3fca427
Showing
3 changed files
with
62 additions
and
0 deletions.
There are no files selected for viewing
This file contains 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
This file contains 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
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(); |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
'use strict'; | ||
|
||
// This leaks because of the strong persistent v8::Module references | ||
// from ModuleWrap. We need replace that with a GC-aware ModuleWrap -> | ||
// v8::Module reference to fix this. | ||
// See: https://github.com/nodejs/node/issues/33439 | ||
|
||
require('../common'); | ||
const assert = require('assert'); | ||
|
||
// We use a child process here because known_issues doesn't work with | ||
// hard crashes. We could just test this block with the flags when | ||
// it's fixed. | ||
if (process.argv[2] === 'child') { | ||
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(); | ||
} else { | ||
const { spawnSync } = require('child_process'); | ||
const child = spawnSync(`${process.execPath}`, [ | ||
'--experimental-vm-modules', | ||
'--max-heap-size=20', | ||
__filename, | ||
'child'] | ||
); | ||
assert.strictEqual(child.status, null); | ||
} |