VM's context when evaluating ESM script not garbage-collected (works with CJS)

[enisdenjo]

Version

latest of 18, 20, 21

Platform

GitHub Actions on ubuntu-latest (and any other platform)

Subsystem

No response

What steps will reproduce the bug?

See https://github.com/enisdenjo/node-leak for repro. Also its failing CI for running repro.

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

Always.

What is the expected behavior? Why is that the expected behavior?

VM's context gets garbage-collected when evaluating both CJS and ESM scripts.

What do you see instead?

VM's context doesn't get garbage-collected when evaluating ESM script. While with CJS it does.

Additional information

No response

[joyeecheung]

If you take heap snapshots before and after you set the ctx = null you can actually see that the context can be GC'ed.

The leak detector is not implemented in a reliable way:

1. The invocation of FinalizationRegistry callbacks is not an indication of whether an object can be garbage collected. See the explainer of the original TC39 proposal which explicitly warns about this https://github.com/tc39/proposal-weakrefs#another-note-of-caution - we also know from experience that when ephemeron GC is involved (e.g. when WeakMap is used, which is the case for vm script/modules) it's easy to produce false positives in leak detection that relies on the invocation of FinalizationRegistry callbacks.
2. --expose-gc is not meant to "clean up the heap thoroughly", in fact it can actually prevent code from aging thus making certain scripts immortal. This is a V8 testing flag so what it does only serves V8's own use cases in tests, but not for general usage https://bugs.chromium.org/p/v8/issues/detail?id=12198#c4

I see that the leak detector seems to come from an old implementation in Jest, I ran into a similar false positive in their previous implementation too, the workaround they did was generating a heap snapshot to "force" a GC: #48510 (comment) (I think this is still somewhat unreliable though, as both 1 and 2 are still true with that approach, it just lowers the chances of false positives but does not eliminate it, though in Node.js core we've still encountered false positives with a similar approach).

[enisdenjo]

@joyeecheung thank you for the thorough explanation! Much appreciated.

[enisdenjo]

@joyeecheung one more question if I may, why does the leak-detector work "reliably" for CJS scripts?

[joyeecheung]

It may just happen to “work” with the given graph (e.g. somehow this leads to enough pressure at the right time for the GC to run in a particular way to get the callback invoked) but it could very well stop working when the implementation of vm.Script or V8’s GC change in a subtle way, which is common for GC-related issues