Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add getUserSharedBuffer() callback finalizer to fix memory leak #323

Closed
wants to merge 1 commit into from

Conversation

cb1kenobi
Copy link
Collaborator

When getUserSharedBuffer() is called with a callback, the callback is added to an internal std::unsorted_map. As far as I can tell, the callback function ref is not GC'd until the env is closed or Node.js exits. We can tie the life of the callback to the life of the returned ArrayBuffer.

If we call getUserSharedBuffer() in a loop, it will consume memory until Node.js runs out of memory on the heap and crashes. Note that the loop needs to yield in order for GC to collect the unused returned shared buffer.

let consumedMemory = 0;
for (let i = 0; true; i++) {
	consumedMemory += 8;
	db.getUserSharedBuffer(`foo${i}`, new ArrayBuffer(8), {
		envKey: true,
		callback: () => {
			console.log('user shared buffer callback');
		}
	});
	await new Promise(resolve => setImmediate(resolve));
}

Before this fix, Node.js would crash after 37M iterations. After this fix, Node.js will not crash.

Further more, if we cap the iterations to 4,222,600 (arbitrary, non-magical number), the code above will have consumed 32.2MB of memory, however we can see the V8 function ref is not being released.

BEFORE
# keys: 4222600  Size: 32.2 MB   ####------------------------------------   457.3 MB / 467.1 MB / 4 GB

AFTER
# keys: 4222600  Size: 32.2 MB   ----------------------------------------    18.9 MB /  43.3 MB / 4 GB

@cb1kenobi cb1kenobi marked this pull request as draft January 22, 2025 16:16
@cb1kenobi
Copy link
Collaborator Author

This PR is misguided. Closing.

@cb1kenobi cb1kenobi closed this Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant