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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit parallel file reads to prevent "EMFILE: too many open files" error #4170

Merged
merged 7 commits into from Jul 9, 2021

Conversation

schummar
Copy link
Contributor

@schummar schummar commented Jul 6, 2021

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

Description

I (and many others, I imagine) have an issue when bundling a big number of imported files:

[!] Error: Could not load /home/marco/src/rolluptest/node_modules/@material-ui/icons/esm/RoundedCornerRounded.js (imported by node_modules/@material-ui/icons/esm/index.js): EMFILE: too many open files, open '/home/marco/src/rolluptest/node_modules/@material-ui/icons/esm/RoundedCornerRounded.js'

It can be reproduced with something as simple as

import { AccessAlarm } from "./node_modules/@material-ui/icons/esm/index.js";

(index.js reexports over 5000 files)

npx rollup 1.js --file 2.js

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 build and 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 馃槈

@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #4170 (4891c10) into master (b3d5f7d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage 螖
src/utils/fs.ts 76.47% <酶> (+5.88%) 猬嗭笍
src/utils/options/mergeOptions.ts 100.00% <酶> (酶)
src/ModuleLoader.ts 100.00% <100.00%> (酶)
src/utils/options/normalizeInputOptions.ts 100.00% <100.00%> (酶)
src/utils/queue.ts 100.00% <100.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update b3d5f7d...4891c10. Read the comment docs.

@schummar
Copy link
Contributor Author

schummar commented Jul 7, 2021

Can someone please approve the workflows? I have added some tests.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

An alternative here would be to use graceful-fs - https://www.npmjs.com/package/graceful-fs.

src/ModuleLoader.ts Outdated Show resolved Hide resolved
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.

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).

src/rollup/rollup.ts Outdated Show resolved Hide resolved
src/utils/options/normalizeInputOptions.ts Outdated Show resolved Hide resolved
test/misc/queue.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@schummar
Copy link
Contributor Author

schummar commented Jul 8, 2021

Also added documentation now. Is it sufficient?

And what do you think about the default maxParallelFileReads value of 20?
My reasoning was, that more likely does not make sense in almost all cases and it's low enough so that you can run quite a lot of rollup instances at the same time without running into those "EMFILE: too many open files" errors.
The number is a bit arbitrary of course, so does someone have a reason to choose another number?

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?

@lukastaegert
Copy link
Member

lukastaegert commented Jul 9, 2021

Also added documentation now. Is it sufficient?

馃憤

And what do you think about the default maxParallelFileReads value of 20?

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.

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 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.

@lukastaegert lukastaegert merged commit ce95197 into rollup:master Jul 9, 2021
@schummar
Copy link
Contributor Author

schummar commented Jul 9, 2021

Ah yes, Windows paths 馃榿
Thanks for the quick reviews and feedback!

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.

None yet

3 participants