Skip to content

Prevent memory leak when using cache, fix leak test - #2522

Merged
lukastaegert merged 3 commits into
rollup:masterfrom
kyle1320:master
Oct 30, 2018
Merged

Prevent memory leak when using cache, fix leak test#2522
lukastaegert merged 3 commits into
rollup:masterfrom
kyle1320:master

Conversation

@kyle1320

@kyle1320 kyle1320 commented Oct 22, 2018

Copy link
Copy Markdown
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
#2494 #1470

Description

Fixes #2494.

#2496 attempted to fix this, but did not appear to delete all problem references. This deletes the additional problem reference.

This can be (and has been) tested manually, in the following way:

(function build(cache) {
  console.log(process.memoryUsage().heapTotal);

  rollup
    .rollup({ cache, /* ...options */ })
    .then(build);
}());

Currently, memory usage will continue to rise until the VM runs out of heap space and crashes. With this fix applied, the GC can do its thing and memory usage stays at an acceptable level.

This test, proposed in #1470, would hopefully prevent this from happening again in the future, but I also don't know how to integrate the test here. Perhaps someone else will be able to.

After more investigation, I realized that the aforementioned test is in fact implemented via npm run test:leak. However, the test uses bundle.cache for the cache instead of just bundle, causing the test to pass even though it should fail.

I've updated the test, and confirmed that it fails before my change, and succeeds after.

@kyle1320 kyle1320 changed the title Delete rawInputOptions.cache to prevent memory leak Prevent memory leak when using cache, fix leak test Oct 22, 2018

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

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.

cache option causes incremental memory usage

2 participants