Handling EMFILE/ENFILE on fs.open

[isaacs]

For build tools that interact a lot with the file system (eg, npm, gulp, grunt, glob, and many other of the most popular node libraries, especially used by the overwhelming majority of "node devs" who are actually using it for front-end asset tooling), it is very useful to have fs.open delay when it hits the process max file limit, rather than fail to open the file. If every file being opened is guaranteed to be closed eventually, "just wait until something closes and then try again" is a pretty good strategy.

The graceful-fs module does exactly this. It is a copy of the fs module, but with the open and close methods fancied up so that it handles EMFILE gracefully.

There are three options to build such a thing. I've explored all three of them, and they have different tradeoffs. All three are being repeatedly broken by recent changes in io.js.

1. Monkey-patch the fs module itself. This is not good because "wait on EMFILE" behavior is not something you want everywhere, so adding it globally is surprising and bad.
2. Clone the fs module. The downside here is that it requires evaling "internal" code. Also, it means that the Stat objects aren't instanceof fs.Stat, and lots of other weird surprising edges.
3. Write a hand-rolled artisanal copy of the fs module, re-implementing everything in core. This is tedious and wasteful, and just as prone to eventual breakage. It means keeping up with every new addition to fs, but it is technically possible.

To forestall the reader who will inevitably make this claim, I believe that "well, you shouldn't do that, and if you do, you get what you deserve" displays a lack of empathy that is incompetence bordering on malice. If npm and other cli tools stop working, io.js doesn't work, and that will limit its uptake in the real world. We are in this real world where people are doing this, so you can either say "we don't want users using our software", or we can come up with a plan to keep enabling this behavior.

At the very least, we must have a smoke test that actually runs graceful-fs's tests, as soon as possible, and reject changes that break it.

Some forward-looking options to take graceful-fs out of the equation and enable more churn in the fs module without breaking everyone:

1. Add a flag to fs.open, fs.readFile, fs.writeFile, fs.appendFile, fs.readdir, fs.ReadStream, fs.WriteStream (and any others I'm forgetting!) to tell it to catch EMFILE errors, put them in a queue, and re-try the open operation when a fd closes. In effect, put graceful-fs in core, but make it opt-in per open.
2. Explicitly bless either the "monkeypatch" or the "clone" approaches above, and stick to that.
3. Vendor the fs module like we do with streams, so that graceful-fs could require it as a dependency. Then the divergence between "use this internal API for speed" vs "use the external equivalent" could be made explicit and handled with magic comments in the build process or something.
4. Boil the ocean of migrating every module that currently uses graceful-fs to stop trying to do fs operations in parallel and handle EMFILE themselves. (This is not a realistic suggestion, imo, but included here for completeness.)

I'll be at Node Conf this week, maybe some of us could discuss this further in person.

[chrisdickinson]

Add a flag to fs.open, fs.readFile, fs.writeFile, fs.appendFile, fs.readdir, fs.ReadStream, fs.WriteStream (and any others I'm forgetting!) to tell it to catch EMFILE errors, put them in a queue, and re-try the open operation when a fd closes. In effect, put graceful-fs in core, but make it opt-in per open.

We could make this a bit more palatable by also exposing a fs.getGracefulFS() API from the module, that would mirror the current fs API but with graceful defaults selected, that way end users (and the graceful-fs module itself) could replace fs use with a single line: fs = require('fs').getGracefulFS(). We could even accept flags on what errors to retry on, and how many times to retry.

Vendor the fs module like we do with streams, so that graceful-fs could require it as a dependency. Then the divergence between "use this internal API for speed" vs "use the external equivalent" could be made explicit and handled with magic comments in the build process or something.

At least for my time with the project, I haven't seen a lot of success in vendoring core modules out into the ecosystem. The audiences (& their goals) a core module and an ecosystem package serve are different. With the advent of internal modules (yay, we can finally test everything!), I'm less keen on vendoring core APIs.

[Fishrock123]

1. I wouldn't be opposed, let's see what people who are more in-tune with the details think.
2. nada :S (But I'm worried about other potential things that may have to do similar hacks..)
3. I think that would not end up well.
4. Heck no.

I'd be down for discussing at nodeconf.

As another bizarre alt, could it be possible to expose internals though process.binding('natives')? Of course then if people eval build directly ontop of those APIs we make no guarantees at all.
(Edit: as in, expose them to other core modules using them, never directly.)

(Note: this doesn't fix the latest issue, but it does fix the main one.)

Also, chris's idea doesn't sound unfavourable either.

[vkurchatkin]

As an idea: introduce Filesystem constructor that takes low level adapter as an argument (process.binding('fs') for example). Let require('fs') be instanceof Filesystem with bound methods. This would allow implement graceful-fs like functionality, mocks, virtual filesystems, etc.

[MylesBorins]

I quite like Chris's suggestion. I was originally thinking of something similar to how Async vs Sync is handled

fs.readFileGraceful()

Although that is much more verbose than .getGraceful()... and would not shim in as, dare I say, gracefully.

This is an edge case I have hit before and would nice to see it handled in core.

[domenic]

fs.getGracefulFS, or perhaps something a bit less verbose like fs.graceful, sounds great. Let's do that.

To forestall the reader who will inevitably make this claim, I believe that "well, you shouldn't do that, and if you do, you get what you deserve" displays a lack of empathy that is incompetence bordering on malice.

At the risk of derailing the thread, I 100% agree. My takeaway is that anything that is not enforced-by-the-software private is effectively public. We've found this on the web too (nobody listened when we said "don't use webkit prefixed APIs because we want to change those later!"). We should use internal modules, or other things like them, more heavily.

[mafintosh]

@chrisdickinson adding a flag to fs.open, fs.readFile, fs.writeFile, fs.appendFile, fs.readdir, fs.ReadStream, fs.WriteStream sounds simpler to use imo. That also makes it easier for module authors to bubble the option up to a consumer like:

function upperCaseFile (filename, opts, cb) { // call with {graceful: true} if you want to enable that
  fs.readFile(filename, opts, function (err, buf) {
    cb(null, buf.toString().toUpperCase())
  })
}

vs

function upperCaseFile (filename, opts, cb) {
  var readFile = opts.graceful ? fs.getGracefulFS().readFile : fs.readFile
  readFile(filename, opts, function (err, buf) {
    cb(null, buf.toString().toUpperCase())
  })
}

Not to mention I now how to features detect for .getGracefulFS as well to support older versions

[Fishrock123]

I'm not entirely sure how @vkurchatkin's suggestion would work, but it sounds like it could be good. I think we should at least explore it before settling on anything.

[trevnorris]

For the FS constructor thing, that's one of the many lower level APIs I've been prototyping. e.g.:

var file_resource = new FS('/path/to/file');

file_resource.stat(function(err, info) {
  if (err) return;
  this.open('w', function() {
    this.write('blah', function() {
      this.close();
    });
  });
});

That example is horrible in terms of implementation, but gets the point across.

[chrisdickinson]

@mafintosh:

adding a flag to fs.open, fs.readFile, fs.writeFile, fs.appendFile, fs.readdir, fs.ReadStream, fs.WriteStream sounds simpler to use imo. That also makes it easier for module authors to bubble the option up to a consumer like:

Those APIs would grow an option, but fs.getGracefulFS() would expose them with defaults pre-filled.

[bmeck]

@domenic I don't want us to fall into the backwards compatiblity nightmare that browsers face right now by not removing/pointing at people when they do things that are undocumented/marked as private (_)/deprecated etc. As it stands most node programs are executed in environments with application devs/ops controlling the version executing your code unlike a browser. big -1 on node should follow browsers into backwards compat hell.

[domenic]

We're already there, and some people just don't realize it yet.

[bmeck]

@domenic i don't see it, minorly breaking changes have been coming in all over iojs