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: prevent EMFILE error in environment low file descriptors limit #306

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tamoreton
Copy link

npm install can fail with a EMFILE error in certain environments. From the docs on common system errors:

  • EMFILE (Too many open files in system): Maximum number of file descriptors allowable on the system has been reached, and requests for another descriptor cannot be fulfilled until at least one has been closed. This is encountered when opening many files at once in parallel, especially on systems (in particular, macOS) where there is a low file descriptor limit for processes. To remedy a low limit, run ulimit -n 2048 in the same shell that will run the Node.js process.

I've seen it building a Node.js v20 container image on a Kubernetes cluster using buildah, as described here. In my case it's a CI environment where I can't easily do the suggested fix of increasing the file descriptor limit with a flag on the build command, and that seems like a workaround anyway. I've also seen it when building a Node.js v18 container image using podman in GitHub Actions. I'm guessing it's more common in CI environments.

This PR fixes the issue by replacing node:fs with fs-extra. fs-extra uses graceful-fs under the hood, which does an incremental backoff if an EMFILE error is encountered.

Both fs-extra and graceful-fs are intended to be a drop-in replacement for node:fs, but graceful-fs doesn't yet support the fs Promises API (isaacs/node-graceful-fs#160) whereas fs-extra does.

References

Fixes #261

@tamoreton tamoreton force-pushed the fix-emfile-error-during-npm-install branch from 66c04b8 to 79a53c0 Compare January 5, 2025 15:44
@tamoreton tamoreton marked this pull request as ready for review January 5, 2025 15:44
@tamoreton tamoreton requested a review from a team as a code owner January 5, 2025 15:44
@wraithgar
Copy link
Member

We usually take adding new libraries to npm pretty slowly, especially something as low-level as fs mocks. Additionally, we already "monkey patch" fs itself in all of npm.

I think what's needed here is a more holistic assessment of fs/promises in npm to determine if there's a better way to do the same thing for that api.

@tamoreton
Copy link
Author

We usually take adding new libraries to npm pretty slowly, especially something as low-level as fs mocks. Additionally, we already "monkey patch" fs itself in all of npm.

I think what's needed here is a more holistic assessment of fs/promises in npm to determine if there's a better way to do the same thing for that api.

graceful-fs would be preferable to fs-extra because the author is a contributor to this project and, it targets just the problem that we are trying to solve, and it keeps the dependency tree shallower. But sadly it doesn't support the node:fs Promises API.

One option I considered was to make this a feature rather than a bug fix and add a parameter for a fs library, defaulting to node:fs, for dependency injection. But then you'd still need to include fs-extra in the call site (which is still within npm), so it doesn't really solve the problem, it just moves it around.

Another option might be to just write the backoff behaviour into this library directly. But that's arguably more risky than using a well-established library.

Interesting that you already monkey patch fs in npm – does that mean that in theory, #261 shouldn't be happening during npm install?

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.

[BUG] EMFILE error in environment with low file descriptors limit
2 participants