SourceTextModule memory leak

[grosto]

• Version: v14.2.0
• Platform: Darwin Kernel Version 19.4.0
• Subsystem: vm

What steps will reproduce the bug?

run this snippet with --experimental-vm-modules

const {SourceTextModule} = require('vm');

let grow = () => {
  new SourceTextModule(`1 + 1`);
  setTimeout(grow, 100)
}

grow()

How often does it reproduce? Is there a required condition?

Reproducible on every run on my machine.

What is the expected behavior?

SourceTextModule should be cleaned up by GC

What do you see instead?

The SourceTextModule instances never get cleaned up.

Additional information

I think issue is that ModuleWrap is used as a key in WeakMap in vm module, but I am not really familiar with this codebase.

[devsnek]

Yeah modules can't really be individually collected, the entire context they're in has to be collected.

[grosto]

I am not sure I understand what you mean by that. Is this expected behaviour?
Even with contextified object issue still persists.

 const context = createContext({});
 new SourceTextModule('1 + 1', { context });

[guybedford]

@devsnek are there steps we can take to improve module collection for v8? Or do we have to wait for concepts like compartments to drive this work instead?

[devsnek]

@guybedford atm the api (on our side and on v8's side) is architected to not really care about gc'ing modules because they tend to have to live for the lifetime of an application anyway. If we want to support this, we have to basically go through everywhere we ref a module in node and in v8 and ensure they're collectable on a per-module or per-context basis.

As you also brought up, compartments sort of replace the entire vm api anyway, and implementations will have to ensure those can be collected properly, so we could also choose to do nothing.

[justinfagnani]

Possibly related to this, is Node caching SourceTextModules based on their identifier?

We have a loader with its own cache and it seems like sometimes we'll get a module back from new SourceTextModule that's already been linked and eval'ed. We're still working on reducing the issue though.

This is for a server that's creating a fresh VM context for every request, so we want a new module graph and cache. We've been assuming that Node does no caching itself.

[devsnek]

@justinfagnani possibly v8:9968?

[justinfagnani]

@devsnek I honestly can't tell if that's related. From that issue, always invalidating the cache seems like what we want - ie, we're taking care of the caching in our loader and if we call new SourceTextModule() it's because we definitely want a new module.

I guess I don't know what the contract is for SourceTextModule. Should reusing the same source and identifier ever result in a module that's already linked? Even if the linker may resolve imports differently? Should we be trying to vary the identifiers so that they're unique across contexts we create?

edit: I'm asking here only because I'm not sure if we should file a new issue yet.

[devsnek]

@justinfagnani

I guess I don't know what the contract is for SourceTextModule. Should reusing the same source and identifier ever result in a module that's already linked?

No. It sounds like you're hitting v8:9968.

[maslow]

same problem + 1.
i use it in HTTP request, memory growing & growing in every request.

[caub]

@maslow you could memoize it so that calling SourceTextModule with the same sourcecode returns the same instance

[elglogins]

Any updates on this? :(

[bnoordhuis]

Needs someone to come up with a pull request. It should be safe to remove the module from the WeakMap once the "import meta object" and importModuleDynamically() steps have run.

You're still going to have a bad time if you don't link and evaluate the module like in OP's example but as evaluating them is kind of the point of modules that doesn't seem like a big deal.

[Havunen]

@joyeecheung Is there a possibility you could look into this issue please.

[joyeecheung]

The issue in the OP has already been fixed by #48510 (which included a similar test, and, if you take a heap snapshot, you can see that the SourceTextModule can be GC'ed). If you are coming from jestjs/jest#12205, I think you may need a separate issue with a different minimal repro that only uses Node.js.