Repository navigation
deprecate fs.exists post io.js 1.0.0 #257
Description
Activity
Related discussion on joyent/node: fs.exists is not being deprecated in 0.12 nodejs/node-v0.x-archive#8418
One counterargument to your 'this deprecation is super noisy as fs.exists is used everywhere' argument is that it's used incorrectly almost everywhere. :-)
I don't have a strong opinion on whether to keep or revert the deprecation notice - seeing how people are often using fs.exists wrong, it's better when it's gone; on the other hand, incompatibilities between io.js and node.js aren't nice - but deprecation notices can be turned off if updating your code is not an option.
One counterargument to your 'this deprecation is super noisy as fs.exists is used everywhere' argument is that it's used incorrectly almost everywhere. :-)
@bnoordhuis swapping
fs.existsforfs.accessdoesn't solve that issue thoughSo my understanding on the "
fs.existsis used incorrectly" situation is that there are 2 issues:permissions
fs.existsdoesn't guarantee that the user can actually do anything useful with the file i.e. it's writable or readable. This is addressed byfs.accessthrough themodeargument, but unless you actually use this argument, isfs.accessany better thanfs.exists?race conditions
The race-conditions issue is outlined in the node documentation:
In particular, checking if a file exists before opening it is an anti-pattern that leaves you vulnerable to race conditions: another process may remove the file between the calls to fs.exists() and fs.open(). Just open the file and handle the error when it's not there.
i.e. Between an
fs.existscall and some read operation, e.g.fs.readFile, the file could have been removed, thus thefs.existscall doesn't actually guarantee that the file exists. So whatever handling you're doing in the case of a failedfs.exists, you should really attach to anif (err && err.code === 'EEXISTS')inside whatever actual operation you're trying to perform.Without any mechanism for multiple atomic filesystem operations, this issue isn't at all constrained to
fs.existsorfs.access– it's a potential issue any time we're doing sequential filesystem operations, correct? e.g. If I do afs.mkdir, node cannot guarantee that another process hasn't removed or manipulated the permissions of the directory before I try tofs.writeFileinto the created directory.In summary:
fs.accessdoesn't at all solve the race-condition problem outlined in the node documentation (which is actually a problem for many otherfs.*methods too).fs.accessdoes provide a more convenient interface for checking existence & permissions thanfs.stat, and does so without thefs.existscallback signature anomaly. Recommendation is to never usefs.access/fs.existsbefore performing write operation dependent on the value, instead should simply handle problems by switching on thefs.*err.code.Is any of this incorrect or misguided?
@timoxley That's correct. fs.access() is a limited but faster fs.stat() (and can be abused for adding strace-visible logging, but that aside.)
but unless you actually use this argument, is
fs.accessany better thanfs.existsThere're basically two reasons:
- less inviting name
- err-first callback signature
fs.accessdoesn't solve any race condition issues by itself, although it might cause people to think about better ways of handing this stuff.no opinion about 1.0.0 vs post-1.0.0 deprecation here
perhaps we need an intermediate type of deprecation for these deprecations that cause incompatibilities with joyent/node that's a bit gentler—perhaps just limiting to a stderr print once during execution rather than every time it's accessed:
fs.exists() will be deprecated. Use fs.access() instead on io.js.Didn't we have this discussion already here: #103 ?
@rvagg That's how it works. The output from the OP looks like a number of child processes all calling fs.exists() and fs.existsSync().
ah, ok then, well I guess my suggestion here is to handle these things just like we do in the browser.. first person to publish an
fsaccesspackage to npm to work around differences can post it here!@trevnorris @chrisdickinson or @indutny - any chance of just landing nodejs/node-v0.x-archive#8714 to keep things in sync?
AFAICT
fs.existsSync()is being deprecated for something that does something different and does not solve the problem. Wouldn't be wiser to keep both exists andfs.accessSync()?In every other server side oriented language there is an equivalent for
existsand it's synchronous since quite about ever.Weird choice if you ask me, I also wonder how many real-world scenarios had problems becuase of such race condition ( probably docummented in some other linked discussion )
Reacted by Landon SchroppPersonally I don't see how
accessreplacesexistsexcept as for little functionality intersection (i.e. check if file is readable).How do I know write pretty common use-case i.e.
if (!fs.existsSync('compiled.data')) { // compile and save data }
with
accessSync? Is it now needed to wrap it intotry ... catchjust to be sure that file doesn't exist?That's the kind of incorrect use I was talking about earlier. What happens when two processes check for compiled.data at the same time? There is no single good answer because it's a race condition.
I admit the
try...catchis ugly. However, it's trivial, if less convenient, to put it in a separate function. Here was the motivation for switching toaccess():exists()does not follow the error first callback style, which confuses people and causes issues withpromisifyand similar tools.exists()can only check for file existence, whileaccess()can also check for specific permissions (read, write, execute).exists()performs astat()inside of atry...catch. This causes any potentially useful error information to be lost. The fullstat()is also slower, as @bnoordhuis pointed out.
It's also worth reiterating that
exists()has been deprecated, but not removed, and that the deprecation message can be suppressed. Since these methods really shouldn't be used anyway, I don't have a strong opinion on the deprecation. I do think thataccess()is slightly more useful though, and there isn't really a need for both.What happens when two processes check for compiled.data at the same time?
It's important, but not for everyone, so it doesn't make sense to make check more difficult for those who don't need it. When I write developer tools (CLI), I don't really care about several processes as user himself is single-threaded :)
exists()does not follow the error first callback style, which confuses people and causes issues with promisify and similar toolsThis makes sense, but throwing error where all that you want is yes/no boolean, doesn't feel intuitive as well. Even if this extended code would be at least a return value for
accessSyncvariant (and not a thrown error), it could work better and wouldn't require extra wrapping withtry .. catch.+1 let's not show the deprecation warning for fs.exists yet.
- added this to the milestone
on Jan 12, 2015 - added a commit that references this issue
on Jan 12, 2015 FWIW
fs.access{Sync}just got added to joyent/node. nodejs/node-v0.x-archive#8714The discussion on the original node issue and this one assumes that the only reason to check if a file exists is to do something with that file. (The Node.js documentation further assumes that what you want to do is open it.) Sometimes all you want to know is that the file exists. Why can't there be a function that does nothing other than tell you this? I get the race conditions, but it's making things complicated for the sake of saving developers from themselves.
@matthew-dean There's no kernel API that allows this to happen. The simplest check you can use to accomplish the same thing is to use
fs.access()withF_OK.
tl;dr Unless the PR to get
fs.accessmerged into joyent/node gets merged before io.js 1.0.0 hits, I have a feeling this deprecation may cost more than it's worth.Anyone following the recommendation to use
fs.accessinstead is instantly making their code incompatible with node, and for little gain.In addition, this deprecation is super noisy as
fs.existsis (mis)used everywhere.Yes, you can turn the deprecation notices off, but it's not ideal.
What is the upgrade path for a module author whose code would otherwise 'just work' without annoying deprecation notices on both node 0.10, 0.12 and io.js?
Wondering if this deprecation (and any other changes like it) should be introduced after a 1.0.0 release. I'm all for rocking the boat but perhaps it would serve the io.js interests better if there isn't too much turbulence at first.