Limit parallel file reads to prevent "EMFILE: too many open files" error#4170
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4170 +/- ##
==========================================
+ Coverage 98.31% 98.33% +0.01%
==========================================
Files 201 202 +1
Lines 7194 7219 +25
Branches 2108 2111 +3
==========================================
+ Hits 7073 7099 +26
Misses 58 58
+ Partials 63 62 -1
Continue to review full report at Codecov.
|
|
Can someone please approve the workflows? I have added some tests. |
guybedford
left a comment
There was a problem hiding this comment.
An alternative here would be to use graceful-fs - https://www.npmjs.com/package/graceful-fs.
lukastaegert
left a comment
There was a problem hiding this comment.
See my comment with regard to testing, but the most important thing missing is documentation! This one needs an entry in docs/999-big-list-of-options.md (note that options are sorted alphabetically there).
|
Also added documentation now. Is it sufficient? And what do you think about the default maxParallelFileReads value of 20? Having a default of Infinity would be closer to the original behavior, though I cannot imagine the change breaking anything. Or does someone have concerns? |
👍
Was wondering about this myself but I agree with your reasoning: It will likely not make builds slower, but it will make Rollup "just run" on many systems. I would say we can still tweak this default later without the need to make it a "breaking" release. |
lukastaegert
left a comment
There was a problem hiding this comment.
Looks perfect, thanks a lot! I pushed a small change to make the test run on Windows, otherwise if everything checks out, I will merge this one.
|
Ah yes, Windows paths 😁 |
This PR contains:
Are tests included?
Breaking Changes?
Description
I (and many others, I imagine) have an issue when bundling a big number of imported files:
It can be reproduced with something as simple as
(index.js reexports over 5000 files)
I encounter the problem in WSL2, which apparently has a pretty low default setting for the maximum allowed open file handles. But in principal you would encounter the same on any system, if the number of dependencies becomes big enough. And increasing the limit works but is not portable and therefore goes against the spirit of just hitting
npm install && npm run buildand everything works.So I'm thinking, since Node.js does not limit it, rollup should have its own limit to get this under control. Reading more than a few files in parallel would not give any performance advantage anyway, because the bottleneck, the disk, would not profit from it, right?
I was not able to write a test for the issue itself, only for the fix, since I'm just not sure how I would do that, without writing thousands of temp files and relying on system wide settings for the test to fail. I can add some if you can give me a hint...
Let me know what you think, I'll happily refine this PR 😉