Memory leak in `vm.compileFunction` when using `importModuleDynamically` #42080

sokra · 2022-02-22T18:00:20Z

Version

17.5.0, 16.14.0, does not happen in 14.19.0

Platform

Microsoft Windows NT 10.0.22557.0 x64

Subsystem

vm

What steps will reproduce the bug?

// test.js
const vm = require('vm')

const code = `console.log("${'hello world '.repeat(1e5)}");`

while (true) {
  for (let i = 0; i < 30; i++)
    vm.compileFunction(code, [], {
      // comment out the following line to make it no longer leaking memory
      importModuleDynamically: () => {},
    })
  if (typeof gc !== 'undefined') gc()
  console.log(
    Math.round(process.memoryUsage().heapUsed / 1024 / 10.24) / 100,
    'MiB'
  )
}

Run this piece of code with node --expose-gc test.js.

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

Always

What is the expected behavior?

Memory usage should stay constant.

You can also comment out the importModuleDynamically option to see the expected behavior.

Here is what I see:

4.32 MiB
4.34 MiB
4.33 MiB
4.33 MiB
4.33 MiB
4.33 MiB
4.34 MiB
4.34 MiB
4.34 MiB
4.34 MiB
4.35 MiB

What do you see instead?

You will see memory usage increasing in an unexpected way.

Here is what I see:

5.46 MiB
5.54 MiB
5.57 MiB
5.6 MiB
5.62 MiB
5.66 MiB
5.68 MiB
5.71 MiB
5.97 MiB
5.78 MiB
5.81 MiB
5.85 MiB
5.88 MiB
5.91 MiB
5.94 MiB
5.98 MiB
6 MiB
6.03 MiB
6.06 MiB
6.09 MiB
6.12 MiB
6.15 MiB
6.19 MiB
6.22 MiB
6.25 MiB
6.28 MiB
6.31 MiB
6.34 MiB
6.37 MiB
6.4 MiB
6.43 MiB

Additional information

There is more information in this issue: vercel/next.js#34659 (comment)

I think this leak was introduced by this commit: bf2f2b7#diff-c1d48dc599e8281b0be28ab449ea52917f6d4cc0578adb61ae99c631078b1a41R387

This commit could also be related: 89e4b36

Here is the code that causes the leak in my opinion:

node/lib/vm.js

Lines 380 to 382 in 45b5ca8

    
           callbackMap.set(result.cacheKey, { 
        
             importModuleDynamically: (s, _k, i) => wrapped(s, func, i), 
        
           });

Here is another piece of code that could be relevant:

node/src/node_contextify.cc

Line 1197 in 6847fec

CompiledFnEntry* entry = new CompiledFnEntry(env, cache_key, id, script);

And this piece of code:

node/src/node_contextify.cc

Lines 1234 to 1248 in 6847fec

    
           void CompiledFnEntry::WeakCallback( 
        
               const WeakCallbackInfo<CompiledFnEntry>& data) { 
        
             CompiledFnEntry* entry = data.GetParameter(); 
        
             delete entry; 
        
           } 
        
           CompiledFnEntry::CompiledFnEntry(Environment* env, 
        
                                            Local<Object> object, 
        
                                            uint32_t id, 
        
                                            Local<ScriptOrModule> script) 
        
               : BaseObject(env, object), 
        
                 id_(id), 
        
                 script_(env->isolate(), script) { 
        
             script_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); 
        
           }

And there is this documentation about BaseObject: https://github.com/nodejs/node/tree/master/src#lifetime-management

BaseObject also defines a MakeWeak, which is not used in that case, but might be potentially relevant:

node/src/base_object-inl.h

Lines 111 to 130 in 470c284

    
           void BaseObject::MakeWeak() { 
        
             if (has_pointer_data()) { 
        
               pointer_data()->wants_weak_jsobj = true; 
        
               if (pointer_data()->strong_ptr_count > 0) return; 
        
             } 
        
             persistent_handle_.SetWeak( 
        
                 this, 
        
                 [](const v8::WeakCallbackInfo<BaseObject>& data) { 
        
                   BaseObject* obj = data.GetParameter(); 
        
                   // Clear the persistent handle so that ~BaseObject() doesn't attempt 
        
                   // to mess with internal fields, since the JS object may have 
        
                   // transitioned into an invalid state. 
        
                   // Refs: https://github.com/nodejs/node/issues/18897 
        
                   obj->persistent_handle_.Reset(); 
        
                   CHECK_IMPLIES(obj->has_pointer_data(), 
        
                                 obj->pointer_data()->strong_ptr_count == 0); 
        
                   obj->OnGCCollect(); 
        
                 }, v8::WeakCallbackType::kParameter); 
        
           }

The text was updated successfully, but these errors were encountered:

benjamingr · 2022-02-23T11:39:30Z

@nodejs/modules

sokra · 2022-06-01T11:32:48Z

Any progress on this one?

megahypercat · 2022-06-17T14:16:35Z

I wonder if this is what I am experiencing as well. A memory leak with thousands of CompiledFnEntry objects in the "containment" view of devtools.

phawxby · 2022-08-18T07:55:51Z

Per this issue #25424 (comment) and this comment on the underlying V8 issue they blame --expose-gc as causing the problem. To make any progress on this I think the issue needs to be reproducible without the --expose-gc.

I think #44211 is a better repro case for that as it can be reproduced without it.

sokra · 2022-08-23T13:05:26Z

To make any progress on this I think the issue needs to be reproducible without the --expose-gc.

It is, --expose-gc is optional for the repro. You can also run it without (which will make number more unreliable, but the leak is still there).

node --max-heap-size=20 test.js
7.28 MiB
7.36 MiB
7.39 MiB
7.42 MiB
7.45 MiB
7.71 MiB
7.74 MiB
7.77 MiB
7.8 MiB
7.82 MiB
7.85 MiB
...
15.58 MiB
15.63 MiB
15.68 MiB
15.72 MiB
15.58 MiB
15.63 MiB
15.67 MiB
15.72 MiB
15.76 MiB
15.92 MiB
15.95 MiB
15.97 MiB

<--- Last few GCs --->
al[19616:000001CE737A4A50]    38145 ms: Mark-sweep (reduce) 15.7 (18.1) -> 15.5 (17.8) MB, 9.5 / 0.0 ms  (+ 0.2 ms in 3 steps since start of marking, biggest step 0.1 ms, walltime since start of marking 984 ms) (average mu = 0.996, current mu = 0.990) final[19616:000001CE737A4A50]    39115 ms: Mark-sweep (reduce) 16.0 (18.1) -> 15.7 (18.3) MB, 1.9 / 0.0 ms  (+ 0.2 ms in 3 steps since start of marking, biggest step 0.1 ms, walltime since start of marking 970 ms) (average mu = 0.997, current mu = 0.998) final

<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
 1: 00007FF6A8571C0F node_api_throw_syntax_error+182911
 2: 00007FF6A84FF2F6 v8::internal::MicrotaskQueue::GetMicrotasksScopeDepth+67078
 3: 00007FF6A850043D node::OnFatalError+301
 4: 00007FF6A8F921CE v8::Isolate::ReportExternalAllocationLimitReached+94
 5: 00007FF6A8F7D552 v8::Isolate::Exit+674
 6: 00007FF6A8DFF5AC v8::internal::EmbedderStackStateScope::ExplicitScopeForTesting+124
 7: 00007FF6A8DFC7CB v8::internal::Heap::CollectGarbage+3963
 8: 00007FF6A8E04425 v8::internal::Heap::GlobalSizeOfObjects+341
 9: 00007FF6A8E53A9F v8::internal::StackGuard::HandleInterrupts+863
10: 00007FF6A8B1599F v8::internal::DateCache::Weekday+7327
11: 00007FF6A902F871 v8::internal::SetupIsolateDelegate::SetupHeap+558193
12: 00007FF6A8FB3E15 v8::internal::SetupIsolateDelegate::SetupHeap+51733
13: 00007FF629184654

nodejs/node#42080 vercel/next.js#34659 (comment)

joyeecheung · 2023-02-23T16:51:00Z

I have a fix for vm.compileFunction() itself in #46785 - although the module constructors are still leaking, and that'd take a V8 CL to fix, but at least we could fix vm.compileFunction() first. The analysis is posted in #44211 (comment).

Previously in the implementation there was a cycle that V8 could not detect: Strong global reference to CompiledFnEntry (JS wrapper) -> strong reference to callback setting (through the callbackMap key-value pair) -> importModuleDynamically (wrapper in internalCompileFunction()) -> Strong reference to the compiled function (through closure in internalCompileFunction()) The CompiledFnEntry only gets GC'ed when the compiled function is GC'ed. Since the compiled function is always reachable as described above, there is a leak. We only needed the first strong global reference because we didn't want the function to outlive the CompiledFnEntry. In this case it can be solved by using a private symbol instead of going with the global reference + destruction in the weak callback, which V8's GC is not going to understand. PR-URL: #46785 Fixes: #42080 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos · 2023-03-15T14:39:04Z

The fix ended up breaking many people and was reverted in #47101

joyeecheung · 2023-03-15T16:09:13Z

I suspect that the compiled function is not enough to keep the CompiledFnEntry alive, which leads to the use-after-free in #47096, though from only looking at the code it appears that should be, but I'll need to do some debugging to find out if the referrer function is the same as the one we are storing references with.

turtlesoupy · 2023-03-29T13:15:01Z

Any updates on this post-revert? We are burned by this memory leak here constantly

joyeecheung · 2023-03-29T15:07:14Z

@turtlesoupy FYI this is a separate issue from #44211, which can still give you significant leaks if you are creating a new module every time the callback is invoked, and that needs https://chromium-review.googlesource.com/c/v8/v8/+/3172764 to be fixed. I also suspect that to fix the leak here without introducing a use-after-free we also need that V8 CL too.

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in #46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: #48510 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: #48510 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: #48510 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in nodejs#46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in nodejs#46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in nodejs#46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in #46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. Backport-PR-URL: #49874 PR-URL: #48510 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. Backport-PR-URL: #49874 PR-URL: #48510 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. Backport-PR-URL: #49874 PR-URL: #48510 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in nodejs#46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in nodejs#46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in nodejs#46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in nodejs#46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in #46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: #48510 Backport-PR-URL: #51004 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

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. PR-URL: #48510 Backport-PR-URL: #51004 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: #48510 Backport-PR-URL: #51004 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>

juanarbol added the vm Issues and PRs related to the vm subsystem. label Feb 22, 2022

odeadglaz mentioned this issue Feb 27, 2022

Node 16 Memory leak sindresorhus/import-fresh#23

Open

AndrewFarley mentioned this issue May 29, 2022

Circular dependencies need to be fixed Couchers-org/web-frontend#16

Closed

aungkhant-samsung added a commit to aungkhant-samsung/hot-shots that referenced this issue Sep 15, 2022

Disable dynamic import for unix-dgram causing memory leak

a0ee55d

nodejs/node#42080 vercel/next.js#34659 (comment)

alan-agius4 mentioned this issue Oct 6, 2022

feat(@angular/cli): add support for Node.js version 18 angular/angular-cli#24026

Merged

feedthejim mentioned this issue Dec 13, 2022

Bug: extremely high memory usage with next dev vercel/next.js#42514

Closed

1 task

This was referenced Feb 22, 2023

vm: fix leak in vm.compileFunction when importModuleDynamically is used #46785

Closed

vm: memory leak on vm.compileFunction with importModuleDynamically #44211

Closed

legendecas closed this as completed in 986498b Mar 1, 2023

targos mentioned this issue Mar 15, 2023

Revert "vm: fix leak in vm.compileFunction when importModuleDynamically is used" #47101

Merged

targos reopened this Mar 15, 2023

lukeapage mentioned this issue Jun 2, 2023

[Bug]: Memory consumption issues on Node JS 16.11.0+ jestjs/jest#11956

Closed

CodingDive mentioned this issue Jun 2, 2023

Bug: Memory Leak in yarn test serlo/backlog#204

Open

M-a-c-Carter mentioned this issue Jun 2, 2023

ESM memory leaks involving jest (upstream bug) #48303

Closed

joyeecheung mentioned this issue Jun 21, 2023

module: rework of memory management in vm APIs with the importModuleDynamically option #48510

Closed

joyeecheung mentioned this issue Mar 13, 2024

module: support require()ing synchronous ESM graphs #51977

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory leak in `vm.compileFunction` when using `importModuleDynamically` #42080

Memory leak in `vm.compileFunction` when using `importModuleDynamically` #42080

sokra commented Feb 22, 2022

benjamingr commented Feb 23, 2022

sokra commented Jun 1, 2022

megahypercat commented Jun 17, 2022

phawxby commented Aug 18, 2022

sokra commented Aug 23, 2022 •

edited

Loading

joyeecheung commented Feb 23, 2023

targos commented Mar 15, 2023

joyeecheung commented Mar 15, 2023 •

edited

Loading

turtlesoupy commented Mar 29, 2023

joyeecheung commented Mar 29, 2023

Memory leak in vm.compileFunction when using importModuleDynamically #42080

Memory leak in vm.compileFunction when using importModuleDynamically #42080

Comments

sokra commented Feb 22, 2022

Version

Platform

Subsystem

What steps will reproduce the bug?

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

What is the expected behavior?

What do you see instead?

Additional information

benjamingr commented Feb 23, 2022

sokra commented Jun 1, 2022

megahypercat commented Jun 17, 2022

phawxby commented Aug 18, 2022

sokra commented Aug 23, 2022 • edited Loading

joyeecheung commented Feb 23, 2023

targos commented Mar 15, 2023

joyeecheung commented Mar 15, 2023 • edited Loading

turtlesoupy commented Mar 29, 2023

joyeecheung commented Mar 29, 2023

Memory leak in `vm.compileFunction` when using `importModuleDynamically` #42080

Memory leak in `vm.compileFunction` when using `importModuleDynamically` #42080

sokra commented Aug 23, 2022 •

edited

Loading

joyeecheung commented Mar 15, 2023 •

edited

Loading