-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Handling EMFILE/ENFILE on fs.open #1941
Comments
We could make this a bit more palatable by also exposing a
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. |
I'd be down for discussing at nodeconf. As another bizarre alt, could it be possible to expose internals though (Note: this doesn't fix the latest issue, but it does fix the main one.) Also, chris's idea doesn't sound unfavourable either. |
As an idea: introduce |
I quite like Chris's suggestion. I was originally thinking of something similar to how Async vs Sync is handled
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. |
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. |
@chrisdickinson adding a flag to 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 |
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. |
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. |
Those APIs would grow an option, but |
@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. |
We're already there, and some people just don't realize it yet. |
@domenic i don't see it, minorly breaking changes have been coming in all over iojs |
@domenic to put this in perspective, the breaking change here is from an implementation detail of the code calling another piece of the code changing. Not the change in a public or even private API signature. If we want to respect that level of compatibility we need to stop using any builtin implicit things and Function.call/.apply and cache those suckers. Which is not only unreasonable, but generally insane to respect function implementation details if we ever patch broken things and change the side effects. |
Thanks for picking this up @isaacs ! The beauty of graceful-fs was always that you could just enable it and did not have to change your code at all. I would love to see something similar in io.js. Why not have a method on the fs module to enable this behavior? fs.setGraceful(true) and you are all set. |
Jeez, loaded language much? That's no way to start a discussion. |
@chrisdickinson et al. Flags plus a "return a copy of fs with graceful by default" option would be ideal. graceful-fs could detect this option, and fall back to cloning for platforms that lack it. I might have some time soon to port the relevant bits of graceful-fs to core in this fashion. (Bikeshed: I think @bnoordhuis I apologize for my judgmental and defensive language in the OP. I am not interested in getting into a debate over whether or not user experience is a priority. My goal was to prevent that discussion so that we could instead have the discussion about how best to serve our users, with "user experience is valuable" taken a priori. How would you have worded it? (Also: since it's not directly relevant to this issue, perhaps we could create a new issue about how we want to start discussions and keep them focused? It's a real problem that blocks a lot of valuable input into this project.) |
Wouldn't it be |
@trevnorris It's not a specific number of ms to delay. The operation gets added to a queue, and re-attempted when a file descriptor is closed. I guess |
@isaacs and is it queued just once, or as many times as it takes to open the file? |
@trevnorris potentially infinity times, though that's hecka unlikely. Since the open attempt happens immediately on the close of some other FD, the chance of getting an EMFILE again is super rare. It would imply a race with another thread pool open() call, which is rare even once, let alone repeatedly. It would not race with other previously-queued EMFILE opens (because queue), so you don't have a thundering herd problem where two open()s keep blocking each other forever. We could of course put the queue deep enough in the internals that we avoid that race, but I don't think it's that important. If you opt into slow-but-steady open()s, then you are deciding that you'd rather just wait longer than manage the fd open queue yourself, and you are expressing a certain degree of confidence that eventually you'll fall below the ulimit. |
I don't have any big opinion on the general idea, but for implementation, please don't use a global flag ( |
From this discussion, seems the interface would need to change to
@isaacs I assume you don't expect |
@isaacs I assume that if node fs supported a |
Let me explain why anything other than a global API is bad. If there are really specific modules that rely on the EMFILE error (unlikely) this would have already been a problem with graceful-fs. Given the number of downloads of gracful-fs in the last month (10.032.306), I think its more likely that those specific modules fix their stuff than people willing to stop using graceful-fs at all. Also, new API means modules have to adopt it. If I find the EMFILE issue in a library XYZ (lets say a glob module that aggressively walks the filetree) that is 2 months old and not maintained anymore, I am lost with this error unless I start forking the module and fixing it to use the graceful = true option. And even if I can fix it, it is an incompatible change to node.js, so my module can only be used in io.js from now on. Keep the graceful-fs semantic, make it opt-in and static. |
I'm totally on board with @vkurchatkin's suggestion of a var util = require('util')
var fs = require('fs')
var Filesystem = fs.Filesystem
var MyFs = function (parentFs) {
Filesystem.call(this)
this._parentFs = parentFs
}
util.inherits(MyFs, Filesystem)
MyFs.prototype.open = function () {
// hijack args, insert own callback, use this._parentFs.open(), check for EMFILE, do fancy queueing stuff
}
var gracefulFs = new MyFs(fs) We've used this approach all across the level* ecosystem with great success. If it's simple enough it's also very extensible and should make the job of graceful-fs (et. al.) much easier. |
You don't have to patch the module itself, do you? You can just wrap it, making it opt-in in the process. const fs = require('fs');
module.exports = Object.create(fs);
module.exports.open = openThatDealsWithEMFILE;
module.exports.createReadStream = createReadStreamThatDealsWithEMFILE;
// etc. Are there downsides to that approach?
That's too much policy, too little mechanism to my liking. |
To be clear, this makes |
@domenic well, it requires new public API, so it should be thought through. IMO |
I'm going to bracktrack a little here. I think @isaacs 's major point was actually this:
I think that testing common io.js npm modules as part of the test suite to make sure new releases don't break stuff is a very positive first step. While the API is committed to the documentation I doubt anyone here is under the illusion that we can freely break express, or lodash and "get away" with it in the release. I'd be very interested in exploring these smoke tests more. Making them in a solid way that survives library authors themselves updating the library is an interesting challenge - I think it might even be feasible to ask library authors to do something like:
What do you think? |
@bnoordhuis What you describe is just a more detailed version of my option (3) above. It means re-implementing everything in fs that can potentially use a file descriptor. It's possible, but a lot more code, prone to breakage, and difficult to keep up to date. Furthermore, without knowing when @brendanashworth No one is suggesting a global flag. @bpasero @sam-github Yes, @trevnorris No, |
@isaacs Disclaimer: this might be an edge case, but... |
Folks are working on this over in build. One nice thing we have if we're just selecting a few packages is that we can cherry pick ones that have working I worry about designing an entirely new API for fs to solve this problem; it seems like we'd have to select a lower-level set of operations to support the existing fs API, which sits about at the |
@chrisdickinson awesome, thanks! |
@isaacs Monkey-patching brought me here. Please note that monkey-patching is not safe from breaking in minor versions, because introducing new functionality could go around monkey-patching. Also, internal re-design that does not change any API behaviour could break monkey-patching. For example, see tschaub/mock-fs#43 — |
See #2026 (comment), Closing. Internal modules will re-land in |
Is this still an issue? I just got this error. ENFILE using |
@chovy Use graceful-fs. |
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 theopen
andclose
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.
Stat
objects aren't instanceoffs.Stat
, and lots of other weird surprising edges.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:
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.I'll be at Node Conf this week, maybe some of us could discuss this further in person.
The text was updated successfully, but these errors were encountered: