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

Bundle tools should handle EMFILE gracefully #1593

Closed
kriskowal opened this issue May 23, 2023 · 3 comments
Closed

Bundle tools should handle EMFILE gracefully #1593

kriskowal opened this issue May 23, 2023 · 3 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@kriskowal
Copy link
Member

Bundling opens a lot of concurrent file descriptors, sometimes more than are available, especially when running the bundler concurrently. We should address this in general, but specifically for the bundle-source tool.

Tools like bundle-source currently construct their file system capabilities from 'fs' module, which does not gracefully handle EMFILE or EAGAIN. The graceful-fs package in npm takes care of these concerns but does not provide a promise abstraction. We would need to lower the compartment-mapper node-powers module a layer of abstraction and embrace fs callbacks directly to use graceful-fs as a drop-in replacement for fs.

Alternately, we should implement graceful handling of these errors ourselves in the compartment mapper. This would implicitly improve other existing applications, since the concern of attenuating an fs power is currently deferred to the individual application entrypoints.

The author of this issue notes that we could have the best of all worlds had graceful-fs been implemented as a powerless adapter and did not import fs directly.

@kriskowal kriskowal added the good first issue Good for newcomers label May 23, 2023
@mhofman
Copy link
Contributor

mhofman commented May 24, 2023

Famous last words but I wouldn't expect handling these errors directly from native fs/promises API to be a too difficult.

tgrecojs added a commit to tgrecojs/endo that referenced this issue Jun 3, 2023
* added Either type for extendable, expressive error handling.

* updated read function behavior with demonstration code showing how to gracefully handle EMFILE errors. unsophisticated approach which simply calls read once again with the same location argument it began its execution with.
@warner
Copy link
Contributor

warner commented Jun 22, 2023

Note to self: the particular feature of graceful-fs that would help here is:

Queues up open and readdir calls, and retries them once something closes if there is an EMFILE error from too many file descriptors

That would enable complete usage of the finite number of FDs. The basic "limit parallelism" scheme that I first had in mind would require us to guess how many we could use, and our guess would likely either exceed the environment's limit, or leave some number of FDs unused (nominally leaving some parallelism opportunities, and perhaps thus performance, on the table).

That said, using EMFILE-retried open() calls for all files means that opening the final output file might block until some other input file had closed, and perhaps we can't close that input file until we've copied its contents into the output file. To avoid a livelock, we might want to open the (single?) output file first, before there's much chance of contention.

And, we might still want to limit parallelism of the bundleSource() call, for the sake of other processes taking place in the user account at the same time (which are not prepared to be as graceful as bundle-source, and would probably crash instead of retrying themselves). The FD limit is a shared resource, and consuming all of them in a single process is a tad impolite. I cannot readily imagine measurable performance benefits to be gained by using more than 1000 open files.

@kriskowal
Copy link
Member Author

Fixed by #1697.

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

Successfully merging a pull request may close this issue.

3 participants