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

fix(cache): add limit for PackumentCache.heapLimit #7589

Open
wants to merge 1 commit into
base: latest
Choose a base branch
from

Conversation

dndred
Copy link

@dndred dndred commented Jun 8, 2024

The problem

The current implementation of PackumentCache gets heapLimit value from the node runtime. But, in bun, the heap_size_limit: Infinity by default, and it seems there is no way to limit it.
Then, the heapLimit value passes to lru-cache cache with doesn't work with unlimited cache size at all.

Trace from lru-cache:

361 |         // NB: maxEntrySize is set to maxSize if it's set
362 |         if (this.maxEntrySize !== 0) {
363 |             if (this.#maxSize !== 0) {
364 |                 console.log('this.#maxSize', this.#maxSize)
365 |                 if (!isPosInt(this.#maxSize)) {
366 |                     throw new TypeError('maxSize must be a positive integer if specified');

Solution

Let's set a reasonable maximum to the heapLimit to avoid this.

@dndred dndred requested a review from a team as a code owner June 8, 2024 14:51
@wraithgar wraithgar changed the title fix(cache) add limit for PackumentCache.heapLimit fix(cache): add limit for PackumentCache.heapLimit Jun 10, 2024
@wraithgar
Copy link
Member

Is there no other value available to us that lets us make a reasonable guess here? 16GB seems quite large. My system is only a few years old and that value is 4GB for me.

If we have to set a default we're going to want to err more on the side of working for more people, so having any other signal that lets us get closer to a value that will work on a particular system is helpful.

@Nichebiche

This comment was marked as spam.

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.

3 participants