Sitelet https://github.com/nodejs/node/issues/50964
Skip to content

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

Description

@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

Activity

  1. joyeecheung commented on Nov 30, 2023

    @joyeecheung
    Member

    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).

  2. enisdenjo commented on Nov 30, 2023

    @enisdenjo
    Author

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

  3. enisdenjo commented on Nov 30, 2023

    @enisdenjo
    Author

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

  4. joyeecheung commented on Nov 30, 2023

    @joyeecheung
    Member

    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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    invalidIssues and PRs that are invalid.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions