vm: Script importModuleDynamically memory leak

[guybedford]

• Version: master
• Platform: Linux
• Subsystem: VM

While investigating #21573 I was interested in understanding why we are storing the importModuleDynamically callback in a weak map, as that surely means that it can be GC'd if there is an indeterminate time before the import() function in the module is called.

I may or may not have diagnosed the issue correctly, but the test case is a bug nonetheless:

main.js

const vm = require('vm');

var source = `

setTimeout(() => {
  import('./test.mjs');
}, 1000)

gc();

`;

new vm.Script(source, {
  importModuleDynamically (x) {
    console.log('dynamic callback');
    return import(x);
  }
}).runInThisContext();

Where test.mjs exists as an ES module.

Running with: node --experimental-modules --expose-gc main.js I get the following output:

(node:12832) ExperimentalWarning: The ESM module loader is experimental.
dynamic callback
Segmentation fault (core dumped)

If I comment out the gc() line or run the import() call without a timeout the correct output is provided.

This makes me think we should reconsider the use of weak maps as the storage for the dynamic callback, and rather accept that the dynamic callback should always have a lifecycle that is directly tied to the lifecycle of the module(s) it is referenced by.

[devsnek]

I think this is because there would be no entry in env->id_to_script_map if the script was collected... @nodejs/v8 would there be a way to ref the script object to execution that comes from it? (like microtasks created as a result of that script's execution would ref the script until those microtasks ran, etc)

[guybedford]

Note that I can fix the issue here as well by switching the WeakMap back into a Map.

I have some ideas for refactoring some of this to match the script lifetimes, may see if I can put something together.

[guybedford]

@devsnek ok so it seems like the underlying issue here is that we don't have any idea from v8 when to dispose the import callback function as that would involve knowing when all the microtasks of the top-level execution have run to completion which we can't hook into.

This would require new work from v8 to properly handle.

In the mean time we need a solution.

It seems like our options are:

1. Revert importModuleDynamically from the vm.Script API and only have a single persistent handler for the internal usage.
2. Simply make the script import module dynamically callbacks always persistent, and warn in the documentation that these will incur a memory leak if many different functions are created so that the best practice is to create a single handler for multiple vm.Script creations to avoid this problem.

(2) sounds to me like the best forward-looking solution while the v8 approach is tackled at the same time.

I can work on a PR for the above.

[guybedford]

Hmm there's an even bigger memory leak problem here and that is the following:

1. The problem in this issue thread is that if I set importModuleDynamically on a script, we need to make that callback function persistent in order to ensure it will be available as import() could be called at any time. Keeping these callbacks around for all created scripts is a memory leak.
2. Now importModuleDynamically takes as an argument the vm.Script instance itself, and as a result we should also be making this persistent.

This then leaves us in the situation where all created vm.Script instances are now leaking.

Unless we can get a v8 way to know when all microtasks of a given Script have come to completion, this remains an architectural memory leak in the approach.

[guybedford]

In terms of moving forward here I'd be interested to hear your thoughts @devsnek.

One architecture I was thinking of that could avoid the need for a v8 API here could be to move the importModuleDynamically callback to the vm.createContext function so that the lifetime of the dynamic callback is tied to the lifetime of the context rather, this avoids the memory leak of the dynamic callback itself.

Then instead of having the Script instance provided as the first argument, instead to provide the script ID as the first argument to the callback, where the script ID is available on the new Script instance through script.id. That would stop the segfault / memory leak for long running scripts not having this object necessarily still available.

[devsnek]

@guybedford a handler per context won't allow people to create and use this in the main context, which seems like an annoying limitation.

as for keying by property... we need to store the data on the native side (ContextifyScript/ModuleWrap) to keep the api unforgable.

Can we try tying the lifetime of the v8::Script and v8::Module to the lifetime of the instances? Idea being that they wouldn't be collected until the v8::Script or v8::Module is collected

[joyeecheung]

so that the best practice is to create a single handler for multiple vm.Script creations to avoid this problem

What would the API look like? Storing the function in advance and use it in multiple vm.Script calls? Having a restriction like this in the API design feels pretty odd to me, I'd prefer we remove this out of the public API surface while we still can (because the whole ESM subsystem is experimental) and figure this out before making it public again.

[joyeecheung]

On a second thought....what's the use case of being able to customize importModuleDynamically for different vm.Script in the first place? Would a user even want to have different behaviors across different vm.Script invocations? Can't that setting be more global instead of being a property of the option object? Take the call site in Module.prototype._compile for example:

node/lib/internal/modules/cjs/loader.js

Lines 685 to 688 in eb664c3

	importModuleDynamically: experimentalModules ? async (specifier) => {
	if (asyncESM === undefined) lazyLoadESM();
	const loader = await asyncESM.loaderPromise;
	return loader.import(specifier, normalizeReferrerURL(filename));

Here we use different functions for different scripts but only because we need filename in the closure, but we can just obtain that from the Script object passed to the callback, we don't have to use a closure.

The leak is caused by us trying to work around HostDefinedOptions being a PrimitiveArray, I'd think the potential leakage is why it's a PrimitiveArray in the first place. In the V8 API HostImportModuleDynamicallyCallback is per-isolate, why can we follow that design and make the setting more global if that's what V8 provides?

[devsnek]

@joyeecheung for one thing it prevents you from using them in the main context, since the handler for that context would be owned by the internal esm loader. It also means you can't create your own forms of virtual module maps without using new contexts which can be very very unwanted since different contexts have unique primordials.

I have a theory that we can fix this by inverting the lifetime relationship of v8::Module and ModuleWrap (and v8::Script and ContextifyScript), but I haven't gotten around to testing it yet.

[guybedford]

The leak is caused by us trying to work around HostDefinedOptions being a PrimitiveArray, I'd think the potential leakage is why it's a PrimitiveArray in the first place. In the V8 API HostImportModuleDynamicallyCallback is per-isolate, why can we follow that design and make the setting more global if that's what V8 provides?

Great point, although I'm not sure I follow the logic of why it needs to be a primitive array. Surely HostDefinedOptions needs to be disposed once it becomes unreachable so that those primitives can be disposed? So if this were permitted to be a normal fixed array, surely that could also call the deconstructor for its elements then too? And if so, that would provide what we needed here ideally.

@devsnek the ID approach would be for a id getter on script or module that returns the internal immutable ID used in the host defined options, so that there is no other lifetime in play required for the dynamic callback apart from the dynamic callback function itself.

[devsnek]

@guybedford i'm not really sure how that approach would fix anything. the apparent issue here is that the referring module is collected, not the module you're returning from the callback. the module you're returning from the callback can't possibly have been collected because you're holding a reference to it so you can return it from the callback.

[guybedford]

the module you're returning from the callback can't possibly have been collected because you're holding a reference to it so you can return it from the callback.

This is a memory leak then as we have to hold the module reference for as long as the dynamic callback exists, which currently is indefinitely.

[devsnek]

also just as a simplification, there's no need to use the vm api directly to reproduce this:

queueMicrotask(() => import(''));
global.gc();

After my --debug build finishes i should be able to work on this a bit more

[guybedford]

As discussed with @devsnek on IRC, we don't currently have a solution to this memory issue without:

A. Making all dynamic import callbacks / vm.Script / vm.Module instances Eternal references (memory leak).
B. Reverting vm dynamic import support.

It does seem like one that goes right down to the base assumptions of how this all plugs together so may need some rethinking.

@devsnek given the above I would like to suggest that we go ahead with reverting vm dynamic import support entirely for now. Would you be ok with that?

[guybedford]

@devsnek could we not just have the exact Script instance stored as a JS object in the host_defined_options here as a fix? that way it would be properly managed by the GC and we don't need to have the weak map / other maps as an indirection.

Surely v8 can allow us to put non-primitive objects in host_defined_options?

[guybedford]

Just checked, and yes, host_defined_options can be any type of JS reference, so that really does sound like the fix to me.

[devsnek]

it's a PrimitiveArray in v8.h, am I missing something?

[guybedford]

Ahh, sorry for the false alarm, I saw Symbol being used in the test and overlooked it is a primitive.

I wonder if it could be extended to be arbitrary data? What would be the constraint there?

[devsnek]

@nodejs/v8 ☝️

[guybedford]

I just tested this case out again and it seems we have managed to make these references persistent already as they are not being GC'd. Perhaps we should just move over to the persistent form explicitly given this is the current behaviour now.

[targos]

@guybedford is this still an issue?

[guybedford]

This is actually a much deeper problem than I originally thought in that V8 itself has no capacity to release module memory without releasing the entire context itself as far as I'm aware.

This is likely an area where Node.js (and Deno for that matter) could lead the way in V8 / spec work to improve as Node.js will be blocked directly on issues like nodejs/modules#527 where users expect to be able to unload modules / refresh the registry.

It could be as simple as a basic functional module.unload() API. But would require some strong v8 collaboration. Perhaps @devsnek will be the one to lead this. //cc @nodejs/v8.

[targos]

@guybedford should we rename this issue / change the op to explain what are the problems now?

I cannot reproduce the segfault anymore.

[guybedford]

Modules are simply leaky in all JS environments (they are never GC'd until the realm itself is), and everyone is aware of it at this point. So I think we can just close this, I just hope we don't assume this is the way things "have to be" as it is clearly not good, and Node.js has a role to push for better.

[kriskowal]

I don’t imagine we can solve this problem and preserve the invariant that import be idempotent, which is important. That being said, with Compartment added to the language, programs will have finer control over the retention of entire module graphs. Releasing a compartment would make all of its module map collectible.