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

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

Merged
merged 3 commits into from Oct 30, 2018

Conversation

kyle1320
Copy link
Contributor

@kyle1320 kyle1320 commented Oct 22, 2018

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
Copy link
Member

@lukastaegert lukastaegert left a comment

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