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

[sokra]

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);
	}

[benjamingr]

@nodejs/modules

[sokra]

Any progress on this one?

[megahypercat]

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]

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]

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

[joyeecheung]

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