deprecate fs.exists post io.js 1.0.0

[timoxley]

tl;dr Unless the PR to get fs.access merged 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.access instead is instantly making their code incompatible with node, and for little gain.

In addition, this deprecation is super noisy as fs.exists is (mis)used everywhere.

fs.existsSync() is deprecated. Use fs.accessSync() instead.
fs.exists() is deprecated. Use fs.access() instead.
fs.existsSync() is deprecated. Use fs.accessSync() instead.

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?

var exists = fs.access ? fs.access : fs.exists // Yuck.

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.

[timoxley]

Related: original PR to deprecate fs.exists #103

[timoxley]

Related discussion on joyent/node: fs.exists is not being deprecated in 0.12 nodejs/node-v0.x-archive#8418

[bnoordhuis]

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.

[timoxley]

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.exists for fs.access doesn't solve that issue though

[timoxley]

So my understanding on the "fs.exists is used incorrectly" situation is that there are 2 issues:

permissions

fs.exists doesn't guarantee that the user can actually do anything useful with the file i.e. it's writable or readable. This is addressed by fs.access through the mode argument, but unless you actually use this argument, is fs.access any better than fs.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.exists call and some read operation, e.g. fs.readFile, the file could have been removed, thus the fs.exists call doesn't actually guarantee that the file exists. So whatever handling you're doing in the case of a failed fs.exists, you should really attach to an if (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.exists or fs.access – it's a potential issue any time we're doing sequential filesystem operations, correct? e.g. If I do a fs.mkdir, node cannot guarantee that another process hasn't removed or manipulated the permissions of the directory before I try to fs.writeFile into the created directory.

In summary: fs.access doesn't at all solve the race-condition problem outlined in the node documentation (which is actually a problem for many other fs.* methods too). fs.access does provide a more convenient interface for checking existence & permissions than fs.stat, and does so without the fs.exists callback signature anomaly. Recommendation is to never use fs.access/fs.exists before performing write operation dependent on the value, instead should simply handle problems by switching on the fs.* err.code.

Is any of this incorrect or misguided?

[bnoordhuis]

@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.)

[rlidwka]

but unless you actually use this argument, is fs.access any better than fs.exists

There're basically two reasons:

1. less inviting name
2. err-first callback signature

fs.access doesn'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

[rvagg]

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.

[benjamingr]

Didn't we have this discussion already here: #103 ?

[bnoordhuis]

@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().

[rvagg]

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 fsaccess package to npm to work around differences can post it here!

[cjihrig]

@trevnorris @chrisdickinson or @indutny - any chance of just landing nodejs/node-v0.x-archive#8714 to keep things in sync?

[WebReflection]

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 and fs.accessSync() ?

In every other server side oriented language there is an equivalent for exists and 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 )

[cjihrig]

fs.exists() and fs.existsSync() have been deprecated in favor of fs.access() and fs.accessSync(), which can do exactly the same thing and more. If you think the access{Sync}() interface is strange, it is (but see #114 for the explanation of why).

[RReverser]

Personally I don't see how access replaces exists except 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 into try ... catch just to be sure that file doesn't exist?

[bnoordhuis]

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.

[cjihrig]

I admit the try...catch is ugly. However, it's trivial, if less convenient, to put it in a separate function. Here was the motivation for switching to access():

• exists() does not follow the error first callback style, which confuses people and causes issues with promisify and similar tools.
• exists() can only check for file existence, while access() can also check for specific permissions (read, write, execute).
• exists() performs a stat() inside of a try...catch. This causes any potentially useful error information to be lost. The full stat() 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 that access() is slightly more useful though, and there isn't really a need for both.

[RReverser]

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 tools

This 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 accessSync variant (and not a thrown error), it could work better and wouldn't require extra wrapping with try .. catch.

[piscisaureus]

+1 let's not show the deprecation warning for fs.exists yet.

[cjihrig]

Closed in 3a85eac

[timoxley]

FWIW fs.access{Sync} just got added to joyent/node. nodejs/node-v0.x-archive#8714

[matthew-dean]

The 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.

[trevnorris]

@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() with F_OK.