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 memory leak caused by caching #38

Merged
merged 1 commit into from Oct 17, 2018
Merged

Fix memory leak caused by caching #38

merged 1 commit into from Oct 17, 2018

Conversation

lephyrus
Copy link
Contributor

@lephyrus lephyrus commented Oct 10, 2018

Short version: Even though the documentation says otherwise, using bundle as config.cache leaks memory and will probably throw an error in the future. Unfortunately, the Rollup documentation does not (yet) reflect this.

Long version:
We've migrated a somewhat sizable library to a Rollup build and use this preprocessor for testing. When watching for changes, Node crashes after typically 2-3 file changes (FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory). While investigating the problem, I found that not passing a cache fixes the problem (but obviously slows down the build a lot).

I then found this issue, for which a fix was released just days ago. I upgraded to Rollup v0.66.5, but the problem remained. Re-reading the issue, passing a previously generated bundle as the config.cache option of a subsequent bundle is apparently bad. It may work, but it's not the intended usage and introduces a memory leak. The documentation will hopefully be adapted and passing a bundle may throw in the future. Instead, bundle.cache should be passed in config.cache. Doing so solves our crashes and works as intended in terms of caching.

Instead of `bundle`, use `bundle.cache` as cache for repeated bundling.
@jlmakes jlmakes added the fix label Oct 10, 2018
@lephyrus
Copy link
Contributor Author

@jlmakes May I ask for a timeline on the next release? This would help me decide if a major PR for a project at work should wait for this fix or go with my fork for the time being. Thanks!

@jlmakes jlmakes merged commit 5b26cdb into jlmakes:master Oct 17, 2018
@jlmakes
Copy link
Owner

jlmakes commented Oct 17, 2018

Thanks for the contributions @lephyrus !!
Check out 6.1.0 (diff: 6.0.1...6.1.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants