Node: gripe: deprecating fs.exists/existsSync

[AkashaThorne]

After noticing in the node docs that fs.exists and fs.existsSync were going to be deprecated, I took at look at the iojs docs to see that it had in fact been.

This is _really annoying_ and seems like it's based out of the assumption that every developer ever is only checking the existence of a file prior to reading/writing in some async context. While I understand the sentiment behind wanting to avoid 'unnecessary abstractions' or race conditions, _this_ is unnecessary.

Whereas I was previously able to handle the checking of a file's existence with a single boolean variable, I'm now given no other option but to either try/catch or make my code async and listen for error events.

This feels like prescriptivism, as I can't think of a single reason why a stern warning of the potential implications and/or examples of caveats to its use wouldn't have sufficed.

Can anyone help me understand why this was necessary (beyond pushing the all-async-all-the-time paradigm (that doesn't always necessarily apply (particularly in the case of synchronous CLI tooling)))?

Or perhaps can I just submit a PR that _un-deprecates_ this perfectly good functionality?

EDIT: _I am happy to provide any additional documentation that is deemed necessary._

[Fishrock123]

This is really annoying and seems like it's based out of the assumption that every developer ever is only checking the existence of a file prior to reading/writing in some async context.

That's not _quite_ how it is.

See fs.exists() in the docs .. it's been replaced by fs.access()

See #114 for more reference.

[Fishrock123]

Reason being: fs.exists() used an API format that was unlike _anything_ else in our API, and could produce results that were unexpected to those who didn't otherwise know.

[AkashaThorne]

fs.access doesn't seem like a replacement given that it also throws when there are any accessibility checks that fail. I don't want/need to do error handling every time I check for a file's existence. I don't think it's fair to call this a replacement.

[Fishrock123]

I'm now given no other option but to either try/catch or make my code async and listen for error events.

That would be correct.

[AkashaThorne]

OK, close the issue before I can even reply? Awesome!

[AkashaThorne]

Deprecation with this kind of logic/motivation is only going to serve to make this platform less hospitable to people doing things outside of what features core is focused on.

I still don't see any reason why thorough documentation isn't an adequate response to any of the concerns that have been mentioned thus far.

[AkashaThorne]

I'm also pretty concerned at the realization that this is how we're treating issues created around valid questions/concerns.

Slapping down an open issue within 20 minutes of its creation is pretty hostile.

[Fishrock123]

Original discussion can be found at https://github.com/joyent/node/pull/8418 & #103

[Fishrock123]

Slapping down an open issue within 20 minutes of its creation is pretty hostile.

The GitHub close issue button is in the same location a cancel button usually is. This isn't uncommon for people to occasionally do. Is the intent not clear since I directly re-opened it? :/

[mscdex]

If you're concerned about having to do a try-catch everywhere for fs.accessSync(), why not just make your own wrapper function(s)?:

var fs = require('fs');
function existsSync(filename) {
  try {
    fs.accessSync(filename);
    return true;
  } catch(ex) {
    return false;
  }
}
function exists(filename, cb) {
  fs.access(filename, cb);
  // or if want exactly the same old API:
 //fs.access(function(err) { cb(err ? false : true); });
}

[AkashaThorne]

or why not leave the wrapper the way it is and let me continue to use fs.existsSync? :smile_cat:

[Fishrock123]

perfectly good functionality?

or why not leave the wrapper the way it is and let me continue to use fs.existsSync?

That same thinking can be applied to just adding more and more sugar to core too. There is plenty discussion in the issues I liked above. At minimum, it wasn't particularly perfect, but it wasn't broken either.

[mscdex]

As previously mentioned, the _majority use case_ for fs.exists()/fs.existsSync() was to check if a file existed _before opening a file_. Removing it and recommending people just use fs.open()/fs.openSync() solves the problems associated with that majority use case.

[AkashaThorne]

@Fishrock123 let's discuss how we can make it good enough to leave in.

[AkashaThorne]

Should I create a new issue, or can we discuss the things that need to be improved on this one?

[bnoordhuis]

This has been discussed to death already. Moving to close.

[AkashaThorne]

@bnoordhuis How about if you're tired of discussing this particular issue: don't involve yourself.

[bnoordhuis]

Kind of hard to do when I get 15 new emails in the 30 minutes I'm away from my computer... at any rate, my point is that you're unlikely to bring up anything that hasn't been brought up before.

[AkashaThorne]

@bnoordhuis understandable. Apologies for contributing to your inbox spam.

I'll take a look at the source, read up on previous conversations, and see what I can do to help fix this.

[sindresorhus]

I made a module for this as there's still genuine use-cases for checking if a file exists without reading the file afterwards.

[ChALkeR]

Btw, what's wrong with fs.access? fs.exists has inconvenient api, for example: https://github.com/petkaantonov/bluebird/issues/418
fs.access in a form fs.access(path, callback) looks to do exactly what fs.exists should have done from the start.
Or am I missing something?

[imyller]

Just stopping by to say that I agree with the point @emilyrose makes here.

It is true that fs.access can fully replace fs.exists and fs.existSync functionality. Especially with wrappers like @sindresorhus kindly implemented. Still many newcomers to the platform will be left looking for simple way to get boolean true/false for file existence as fs.access might not be obvious to them.

I personally think that it is, in some cases, acceptable for io.js core APIs to offer functionality that emphasizes simplicity and rapid-development over formal conventions. fs.exists might belong to that category.

[benjamingr]

@emilyrose the reason it was deprecated is because fs.exists has a broken API - I opened the deprecation issue to either remove or fix this. fs.exists typically also suffers from race condition but you can use fs.access to easily do what you did with exists before or heck - use a userland package there are dozens on NPM.

[benjamingr]

@imyller that sounds like a documentation issue - and a good pull request

[imyller]

@benjamingr that is true. I do agree that fs.access has the correct API and fs.exists has issues, but I think the point of this issue is that there still are solid reasons to have simple file-exists-or-not function returning a boolean.

Additionally, one might want a documentation on why you can create full HTTP server implementation with one-liner, while needing many more lines to check if some local file exists ;)

[rlidwka]

I think the point of this issue is that there still are solid reasons to have simple file-exists-or-not function returning a boolean.

Well you can't even tell if file exists or not with the fs.exists, because in case of an error it'll fail silently even if file does exist. And it does nothing fs.stat can't do.

I don't see a single reason why such an api should exist in the core.

Additionally, one might want a documentation on why you can create full HTTP server implementation with one-liner

__Don't worry, the entire http2 community is working on it. In a year or so you won't be able to create http server without getting ssl certificates, and it will surely not be a one-liner anymore__

[ChALkeR]

_Off-topic_: @rlidwka Well, in several months getting ssl certificates will be a one-liner ;-).

[Fishrock123]

@emilyrose have you had a chance to check out the previous conversations? If you think extra documentation could help, a PR would be appreciated. :)

[AkashaThorne]

@Fishrock123 I have! I definitely have a bit more insight into the reasoning behind such a strong resistance to keeping fs.exists. I still don't see any valid issues against fs.existsSync that can't be resolved with documentation.

I'm still happy to contribute to this documentation (and also whichever API fixes are deemed necessary) to make this something we feel comfortable keeping.

[rotemdan]

Here's a simple example where exists() (or more precisely existsSync()) is used without any chance of race conditions:

I'm developing a REST API where resources are directly mapped from IDs to filenames. Before performing any CRUD operation I check if the file exists and send a "404" response if it doesn't (there isn't always a need to open the file since some of the operations replace its content or delete it).

There is no problem of race conditions since I have a dedicated event loop (implemented as a queue of functions/tasks) for each resource, effectively giving me a "lock" around it. The files are never accessed from outside node.

If existsSync() is removed from the API, I would simply write a polyfill for it that does the same thing, so I honestly don't understand why that should be done in the first place. As for exists() I understand the problem with the non-standard callback parameters, so I guess that should be handled separately.

[benjamingr]

@rotemdan if you're using existsSync inside your server I'd argue you're not using io.js correctly to begin with, and I wonder how your performance is even half reasonable since you're blocking the event loop.

Also, you _do_ have a race condition - what happens when you want to run your server off multiple cores (and use the cluster module) for instance?

[rotemdan]

@benjamingr
The times I measured for existsSync are usually around 100 microseconds (on Windows), if I recall correctly. It would be extremely rare that it would need to reach physical media at all to complete the call, since it only accesses metadata. You might be correct though that in a heavily loaded server it may introduce sub-optimal delays, but these are probably so tiny to be negligible in most cases.

Having multiple processes modifying the same file is not supported as there is currently no synchronization done external to the process. You might think this is significant limitation and perhaps, say a database should be used instead, but the thing is, the application is a database! And it performs extremely well! :)

(I was probably inaccurate to describe it as a "simple" case though..)

Anyway, there are many cases where single threaded blocking non-server application (e.g. scripting etc.) would need to use this call as performance isn't really an issue there. There are situations, for example where a app has a list of files and simply wants to verify they still exist and print an error otherwise. It would be convenient to have a function that does just what it says, especially for novice developers.

There are much worse ways to "shoot yourself in the foot" that can't really be prevented by the API, say, opening multiple instances of a file and reading/writing to it concurrently. Sure, the documentation can always warn you of the dangers of doing silly things, though I doubt that should actually be its role..

Edit: nanoseconds -> microseconds

[trevnorris]

Maybe we just strike middle ground here and move the fs.exists() implementation to use fs.access()? I mean, from what I understand that's what the majority of people are using access() for anyway. At least that way there's near zero maintenance overhead, and we don't break anyone.

[benjamingr]

@trevnorris I don't see what downside there is to deprecating and later removing fs.exists - the major issue with it wasn't the race condition it was the inconsistent signature. The original issue also says "deprecate or fix fs.exists".

I wouldn't even mind a fs.exists2 if people really care about this use so much - it's the fact it has a nonstandard callback convention that bothers me and has bitten me and plenty of other developers before.

The fact using fs.exists is a bad idea is an educational issue and I don't think we should presume to understand everyone's workflows but an inconsistent API is just a poor idea all around.

[trevnorris]

@benjamingr unconventional callback convention? as in how it passes exists as the first callback argument?

[ChALkeR]

@trevnorris For example, petkaantonov/bluebird#418

[benjamingr]

@trevnorris Yes, fs.exists does not have an error argument as the first parameter of its callback.

To be honest most of the time I've seen people fail with it it wasn't promises related or generator or as such (although automatic tooling is a real issue come to think about it). It was just people who were unfamiliar with the API who got bitten from the API since they expected the (err, exists) structure.

[ChALkeR]

@benjamingr Just to be fair, there are tens of thousands of usages for fs.existsSync only on GitHub. And hundreds of thousands of usages for fs.exists or fs.existsSync.

_That's an example of how soft-deprecation does not help, btw._

[benjamingr]

Which is exactly why I'm in favour of its deprecation as it happened and not its removal. That said, I'm not ruling out adding an alternative method with a correct nodeback signature.

[ChALkeR]

The alternative method with a correct signature is fs.access, what's wrong with it?

[Fishrock123]

That said, I'm not ruling out adding an alternative method with a correct nodeback signature.

I don't see how exists(function(err, exists) {}) would make any sense. Why would that ever error?

[ChALkeR]

@Fishrock123 Because that's the function signature convention. See https://github.com/iojs/io.js/issues/1592#issuecomment-100688956 and petkaantonov/bluebird#418.

[Fishrock123]

Because that's the function signature convention.

Right. Since it doesn't make sense either way, I don't see why we'd ever un-deprecate it.

existsSync is a different story, although we may have to forever maintain a deprecation warning for exists because people might think it exists.

[benjamingr]

@Fishrock123

There are cases a hypothetical exists _should_ error in my opinion, to name the first two that come to mind:

• What if the disk was unmounted during the access?
• An invalid path was supplied.

In other languages, it throws for other things - for example in Java exists throws for security but in C# it does not. I really think the convention on its own is a good enough argument though.

[Fishrock123]

@benjamingr Sure, but one could argue:

1. return false, it no longer exists.
2. return false, it does not exist.

[benjamingr]

Right, but what if it's exists but the media you're trying to access is temporarily unavailable? I can easily come up with 10 really weird edge cases involving a FUSE OS, network connectivity issues and so on but I really don't want to be the guy who argues the weird edge case no one in practice cares about.

The only part that's actually bothering me about fs.exists apart from the race condition issue is the fact I've seen people consistently get bitten by it when expecting a different signature.

[imyller]

• Both exists and existsSync already use stat internally. (would access be better somehow?)
• The exists does have a bad API as it does not follow the callback convention. It should be deprecated.

And to put things in perspective:

fs.exists = function(path, callback) {
  if (!nullCheck(path, cb)) return;
  var req = new FSReqWrap();
  req.oncomplete = cb;
  binding.stat(pathModule._makeLong(path), req);
  function cb(err, stats) {
    if (callback) callback(err ? false : true);
  }
};

fs.existsSync = function(path) {
  try {
    nullCheck(path);
    binding.stat(pathModule._makeLong(path));
    return true;
  } catch (e) {
    return false;
  }
};

[Fishrock123]

So, while working on https://github.com/nodejs/node/pull/2356, I noticed that fs.exists*() ignores EPERM errors too. This is a serious problem if something on windows decides to starting causing that. In this case, fs.exists*() will report that the file does not exist, but it might actually still exist.. :/

[sam-github]

The best replacement for fs.exists() isn't fs.access(), its fs.stat(), because while exists causes nasty depreacation warnings and support calls from concerned users, access flat out fails to exist on v0.10... Portable code needs to avoid either.

[trevnorris]

fs.exists() ~= fs.stat() ;-)

[sam-github]

fs.exists() ~= fs.access(), either!

[trevnorris]

@sam-github mix up on math notation. I meant ~= (equivalent to), not ¬= or != (not equivalent to). :-)

[sam-github]

oh, sorry!

[Fishrock123]

Except that it isn't in the case of EPERM. You can actually catch it in those cases.

[ccravens]

I want to add my 2-cents to this discussion as well, because although it is ridiculous at the surface that a language like Node would not offer a simple "exists()" method to determine if a file-system object exists or not with a simple boolean return and we have to waste so many development hours arguing about such a miniscule topic, this is what happens when using statSync() for me:

try {
       var stats = fs.statSync('/path/to/file/that/does/not/exist');
} catch(e) {
      console.log('This runs, however e is undefined!');
      console.log(e);
}

and I get the following output:

This runs, however e is undefined!
undefined
fs.js:0
(function (exports, require, module, __filename, __dirname) { // Copyright Joy

Error: ENOENT, no such file or directory '/home/git/repos/working/56121765929e91e2193aad3b/demo/ae/system.xml'
    at Error (native)
    at Object.fs.statSync (fs.js:797:18)
........

So it appears that by using statSync on an filesystem object that does not exist I now get errors thrown that I can not catch and ugly output that I have to go out of my way now to handle? two thumbs down :( Checking whether something on the file system exists or not should be a very simple thing and removing that capability from a purist standpoint seems to confusing and a waste of everyone's time... For example:

http://stackoverflow.com/questions/4482686/check-synchronously-if-file-directory-exists-in-node-js

or:

http://geoff.greer.fm/2012/06/10/nodejs-dealing-with-errors/

His suggestion is to now add an event listener on the process for handling this?? I'm confused, what IS the correct way to determine if a file or directory exists in the filesystem in Node? The whole point of checking if it exists is because the file may not exist and should be able to handle this appropriately. Apologies if I'm missing something dead simple here, but after seeing this thread it seems like this continues to be an issue?

[ChALkeR]

@ccravens I can't reproduce that.
What is your version and platform?

[ccravens]

RHEL 7, Node v0.12.4

Output is being output to console.error, not console.log as stated above. Still don't understand why e is undefined in the catch() block though.

[ccravens]

@ChALkeR After doing some research and looking at fs.js, I added a process.exit() within the catch and no longer get the ENOENT output. I am doing some slightly non-standard i/o connecting to a remote git process using Node and perhaps there was an issue there with the stack error being shown to the remote git client. It has been resolved that way.

[ChALkeR]

Output is being output to console.error, not console.log as stated above. Still don't understand why e is undefined in the catch() block though.

@ccravens Are you sure that that is undefined in the catch block? Try console.log('From catch: ' + e); instead of console.log(e);.

[ccravens]

very strange it is no longer undefined, now I get this:

From Catch: Error: ENOENT, no such file or directory '/home/git/repos/working/56121765929e91e2193aad3b/demo/ae/system.xml'

which is what I originally had expected before my post

[ChALkeR]

@ccravens Are you are mixing stderr and stdout output?

[AkashaThorne]

So, while working on #2356, I noticed that fs.exists_() ignores EPERM errors too. This is a serious problem if something on windows decides to starting causing that. In this case, fs.exists_() will report that the file does not exist, but it might actually still exist.. :/

@Fishrock123 I'm somewhat certain that this bug has bitten me. Gripe rescinded. :smile_cat:

[Fishrock123]

@emilyrose is it still worthwhile to keep this open?

I want to add my 2-cents to this discussion as well, because although it is ridiculous at the surface that a language like Node would not offer a simple "exists()" method to determine if a file-system object exists or not with a simple boolean return and we have to waste so many development hours arguing about such a miniscule topic, this is what happens when using statSync() for me:

@ccravens that is all fine until you start dealing with windows, which could throw an exception that makes it impossible to tell.

the only reasonable way to completely un-bug the API on windows is to make it return an error if it is EPERM, which is a breaking change too large to justify imo.

[ccravens]

@ChALkeR yes I believe I mixed up the two output consoles and am no longer seeing the stack trace on my side. Also the error object is no longer undefined in the catch statement. Functionality is as expected after exiting the process via process.exit()

@Fishrock123 Yes, agreed and after spending some time on it I can see the reasoning for it. I believe looking at developers coming from other languages the lack of a "typical" exists() method with a boolean return value catches some off guard. And having to construct a full try-catch block and testing the result of the error object seems like a lot of work to those coming into Node.

But with as much as Node has given me in terms of all the NPM packages simple libraries, speed and flexibility, the pros def outweigh the cons of not having an exists() method.

[Fishrock123]

I'm going to close this out. We can re-open if necessary. It's generally a pretty tricky issue. In summary:

• fs.exists(cb) does not conform to the "errback" (cb(err, ...) API style that core uses.
• fs.exists{Sync}() is an antipattern when used before other fs operations and should be discouraged in favor of error checking in the other operation by the user due to race conditions.
• fs.exists{Sync}() completely fails on windows if a file is unavailable for access due to EPERM errors.
• This API is still has deeply rooted usage in the node community and changing (for the forseeable future) it is not reasonable. As far as I can tell, the only reasonable thing to do for EPERM is to throw/pass it on, which would require modifications to this API.

That being said, using fs.access{Sync}() is not the most user-friendly. A user-land module is probably the best solution here. For those that would like an alternative, @sindresorhus has one here: https://github.com/sindresorhus/path-exists

[matthew-dean]

fs.exists

A One Act Play by Matthew Dean

Node: "fs.exists has some issues; most importantly, it's not reliable because of race conditions. We should remove it."
Dev: "I don't have race conditions."
Node: "You will always have race conditions."
Dev: "Here, look at this. No race conditions."
Node: "Well, you may not have race conditions. But the API is broken. That matters to you."
Dev: "It doesn't."
Node: "Well, okay, but it's redundant, because you can do the same thing with this other thing which is more complicated and verbose."
Dev: "I don't want to use that other thing."
Node: "Well, you could also use this other thing."
Dev: "...That's even more verbose."
Node: "Well, okay, but you could use something from NPM."
Dev: "Why would I want to do that? fs.exists exists right now."
Node: "No, you want to do that. You want to do it because the things we suggested to replace fs.exists are more complicated than fs.exists. There's no way you'd want to actually do the things we suggested that you do."
Dev: "Then why not just not remove fs.exists???"
Node: "It's too late. It's gone now. Closing as not an issue since the thing it refers to may no longer exist."
Dev: "Hang on, does fs.exists exist still or not?"
Node: "We don't know. We removed the thing that could easily tell you whether a thing exists. We recommend you install something from npm that will tell you what things in Node exist."
Dev: "..."

The End

[bnoordhuis]

Dev: "Here, look at this. No race conditions."

When, in fact, there _is_ a race condition.

I'm not being facetious. I review a lot of code bases for my day job and fs.exists() is misused more often than not.

[xixixao]

is misused more often than not.

Yeah, I review a lot of code bases and Python is misused more often than not.

(could also have said classes in Java)

I think @Fishrock123 could look again at existsSync - the fact that it is used in an anti-pattern does not mean it does not have legitimate use cases and it does not fail on Windows because if you get those EPERM errors you have a problem anyhow. I think there are two reasons this got deprecated: 1) the legitimate API discrepancy (only affecting exists) 2) focusing too much on writing servers, which is not the sole thing Node is used for today (notably, desktop scripting).

[bnoordhuis]

the fact that it is used in an anti-pattern does not mean it does not have legitimate use cases

While not objectively untrue, the fact that people still get it wrong most of the time, even with the dire warnings in the documentation, is a strong indication that it's a terrible API.

[trevnorris]

For those who never actually looked at exists{Sync}'s implementation, it was effectively:

fs.existsSync = function(path) {
  return !!fs.statSync(path)
};
fs.exists = function(path, cb) {
  fs.stat(path, (err) => cb(!!err));
};

What I'm attempting to demonstrate is that there is no secret magic that made fs.exists{Sync} work. Deprecation of these two methods does not mean a huge chunk of functionality has been removed. In fact they have always been about the most minimal sugar that existed in core.

I'm now given no other option but to either try/catch or make my code async and listen for error events.

If your code is going to be rock solid then this has to happen anyway. While not doing this may work out, it's still a fundamental flaw in the code logic.

it does not fail on Windows because if you get those EPERM errors you have a problem anyhow.

IIRC (@Fishrock123 please correct me) then fs.access{Sync} should work fine.

[matthew-dean]

@trevnorris If that's the code, then this finally makes some sense, if indeed fs.exists calls fs.stat and returns the existence of an error or not.

I think the huge source of confusion (for me, anyway) was the suggestion to use fs.open to determine if a file existed, which is equally arbitrary as recommending that a developer substitute calls to fs.exists with calls to fs.rename or another other random filesystem method which would fail if a file doesn't exist. It doesn't make sense if the action you are trying to perform is only determining a file's existence and not opening or renaming or any other random method. (And does not create a race condition when existence is the only piece of information being retrieved.)

So, the way this was removed was terribly confusing, probably because a solution to replace it wasn't really determined (as evidenced by the number of suggestions to replace it). In addition, part of the explanation was that it wasn't consistent with the API, or usually wasn't used as intended, or was usually misused, all of which are irrelevant and not helpful, so I think that led the discussion off-track.

So, I think a good chunk of confusion around the deprecation / removal of these methods is one of communication.

Is it safe to say then, that in terms of file access, one can only ever determine whether or not a file is accessible or not, and not whether it exists or not? Or will a filesystem report that a file exists but the user doesn't have permission to access it? I'm just trying to determine what information can be gleaned from using fs.access or fs.stat instead.

[benjamingr]

Let me ask you this @matthew-dean, _why_ do you check if a file by a specific name exists?

[matthew-dean]

@benjamingr I have what I would think is a fairly common use case. I have a typical "recent files" list in my app. When the user starts the app again, I (currently) use fs.exists to just quickly "validate" the list. If any file no longer exists, I remove it from the list, to improve the user experience. I have a few other such "file references" that I clean up if the file has been removed in the time before the app runs again.

If/when the user eventually opens the file, sure, I'll have to handle the file possibly not existing at that point, but I would have to anyway, since I need to account for any sort of file access error when opening.

So, in my case, when I call fs.exists, that's all I want to know. I'm not going to do anything else with the file at that time. That, after all, is _what the method is for_. If it's sugar, what of it? The same "sugar" exists in every platform everywhere, because it's useful information. e.g. "I have a file reference. Is it still valid?" It's not used to immediately pre-empt another file operation, and I'm not sure why anyone would use it that way. If it's possible a new developer to Node might think they should call it before doing something like opening, and that _might_ cause an issue (which, who the f cares, they'll probably still need to handle file errors, including missing file ones), then you might note that in the docs. But then, that's true for any successive file operation, where the stats on a file may have changed by the time you go to operate on it.

I mean, after all, fs.watch and fs.watchFile aren't getting deprecated or removed, even though the docs say "The fs.watch API is not 100% consistent across platforms, and is unavailable in some situations" which translates as "never ever trust this to work", and watching files via node is known to be so unreliable that no one ever uses them directly. The whole path API isn't getting removed even though it's mostly sugar for simple string replacements.

So I don't get it. I don't get why this one, why it's so important to pull out. Why is there so much dedication to its removal despite valid use cases? If it sucks in some ways, then treat it like fs.watch and say "this may bite you sometimes". I don't want to keep harping on it, because I know as maintainers it sucks to have to explain everything in detail, and you have a greater overview of platform than probably any individual user, but...that's just like, my opinion, man.

[ChALkeR]

@matthew-dean fs.exists is not better in any way than fs.access. Just use fs.access, which has a correct API (fs.exists doesn't).

[matthew-dean]

@ChALkeR Fair enough. I won't belabour the point more. I was just noting more my personal confusion over the matter, which doesn't imply that the removal is wrong; just that I personally didn't understand it. If the onus is on me to understand the use of fs.access, that's also fair. Like I said, probably just a communication gap more than anything.

[Fishrock123]

IIRC (@Fishrock123 please correct me) then fs.access{Sync} should work fine.

Both stat and access work fine, just not only checking if an error exists as an indicator the file exists or not.

Perhaps in some future we could just re-add exists as a wrapper ontop of access with a proper API or something. access is, admittedly, slightly more complex to use.

[AkashaThorne]

sorry @Fishrock123, I didn't want to shutdown discussion but I am totally find w/ closing this (as you have).

[ChALkeR]

@Fishrock123 access isn't more complex afaik. accessSync is.

[xixixao]

There is no simple way to get the functionality of existsSync and it is such a common operation that pushing it out of the fs module which provides all the other tools at the same level of abstraction simply doesn't make sense. The docs for accessSync give it away by the way:

Synchronous version of fs.access. This throws if any accessibility checks fail, and does nothing otherwise.

That is a terrible API if I've ever seen one :).

[trevnorris]

@xixixao How is that much different from existsSync?

[xixixao]

@trevnorris existsSync returns a boolean value. That's a pretty standard thing "functions" do, return values. More practically, getting a boolean depending on whether something throws is ugly, unintuitive, and something you won't be doing every time you want that boolean, so you'll wrap it in a function. Since you're on Node you'll put that function into a standalone package, as someone already did. Which means you'll have to manage that dependency every time you need that boolean. The question is, is this functionality something that belongs alongside the rest of the fs api. Checking whether a file exists? I'd say yes. That's my opinion.

[trevnorris]

Just to quickly check, you realize that fs.existsSync() simply wrapped that in a try/catch and returned accordingly?

[xixixao]

Yes. Of course. Clearly, you won't substitute a call to existsSync with this, in place:

var fileExists = true;
try {
  fs.accessSync(filename, fs.F_OK);
} catch (e) {
  fileExists = false;
}
...

(btw the docs of access don't say what is the default mode, had to look at source...)

Also I couldn't find an answer in the two previous issues: So will these functions be removed from a stable module? Not sure what's the practice here besides the definition of

Compatibility with the npm ecosystem will not be broken unless absolutely necessary.

[matthew-dean]

Just to quickly check, you realize that fs.existsSync() simply wrapped that in a try/catch and returned accordingly?

This could be an argument either for or against an API method, depending on the developer. As in:

Dev A: "Do you realize that this is trivial to write yourself?" "Right, that seems unnecessary to keep."
Dev B: "Do you realize that this function exists which reduces how often you have to solve this problem yourself?" "Right, that seems essential to keep."

[trevnorris]

This could be an argument either for or against an API method, depending on the developer.

I wasn't making an argument. Just wanted to verify. :)

[xixixao]

Or perhaps a new sync function should return the stats and null/false otherwise? https://github.com/nodejs/node-v0.x-archive/blob/master/lib/module.js#L86

[dfabulich]

access is better than exists, but existsSync is much more convenient than accessSync.

I use (used) existsSync in a simple shell script. Wrapping accessSync up in a try/catch block is pointless boilerplate.

Here's a modest proposal: undeprecate existsSync only.

[jnordberg]

Soo... Anyone willing to share a existsSync implementation? I'm guessing that try { fs.statSync(path); return true; } catch (e) { return false; } isn't enough since fs.statSync will throw for more reasons than the file not existing.

[jnordberg]

Also, i agree with @dfabulich, why not just deprecate fs.exists and leave fs.existsSync? It seems to me all the problems are with the asynchronous version and everyone who are against this change just want a quick way of checking if a file exists synchronously.

[ChALkeR]

@jnordberg

fs.statSync will throw for more reasons than the file not existing.

fs.existsSync will return false for more reasons than the file not existing in exactly the same way. That was one of the problems with it.

just want a quick way of checking if a file exists synchronously.

That's not possible. You can't distinguish between a non-existing file and non-accessible file in a safe way.

Check https://github.com/nodejs/node/blob/master/lib/fs.js#L220.
One of the major problems with exists/existsSync is that its name gives you a wrong impression of what it actually does.

[jnordberg]

@ChALkeR okay, that's bad. I expected the behaviour would be existsSync() -> true/false or error thrown if it can't be determined. What do you think about fixing the function instead of removing it?

[Fishrock123]

Making it throw an error kind of defeats the point of not just using one of the alternatives.

On Nov 22, 2015, at 9:26 AM, Johan Nordberg [email protected] wrote:

@ChALkeR okay, that's bad. I expected the behaviour would be existsSync() -> true/false or error thrown if it can't be determined. What do you think about fixing the function instead of removing it?

—
Reply to this email directly or view it on GitHub.

[ChALkeR]

@jnordberg Have you read my answer? As I already said:

That's not possible. You can't distinguish between a non-existing file and non-accessible file in a safe way.

Examples: file in a protected directory (no way to tell if it doesn't exist or if you don't have permission to access it), remote disk and network connectivity issues, and so on.

What should your proposed «fixed» existsSync return for those cases?

[jnordberg]

I would expect it to throw. The question we are asking is: "does a file exist at this location?"
and the answer can be one of yes, no and i can't tell you because of X. which in javascript land would be a error thrown.

Here's the function i'm using at the moment. If there is any interest I'll be happy to work on a patch to the fs lib.

var fs = require('fs');

existsSync.__test = (typeof fs.accessSync === 'function') ? fs.accessSync : fs.statSync;
function existsSync(path) {
    try {
        existsSync.__test(path, fs.F_OK);
        return true;
    } catch (error) {
        if (error.code === 'ENOENT') {
            return false;
        } else {
            throw error;
        }
    }
}

console.log(existsSync('.')); // true
console.log(existsSync('nonexistent/file')); // false
console.log(existsSync('/home/someone/.ssh/id_rsa')); // throws EACCES

[jnordberg]

@Fishrock123 I don't agree, it would only throw on edge cases like a protected directory or some other file system problem that you would want to know about anyways. You wouldn't need to wrap it in a try/catch for normal usage

[Fishrock123]

it would only throw on edge cases like a protected directory or some other file system problem that you would want to know about anyways.

I've thought about this and am sympathetic to it, but still, making something throw is pretty large change, especially for something in as wide of use as exists{Sync}().

Perhaps a better way would just make it return an error or true? (This is still an even bigger change..)

[matthew-dean]

I wouldn't throw an error, since as you mention, that breaks things and is as verbose as any other method. I would do as some PHP functions do. Return boolean true or false when things are knowable, or an integer (or err object) representing an error. Then the only change would be from if(fs.existsSync('file')) to if(fs.existsSync('file') === true)

[ChALkeR]

@matthew-dean Which would equally break the whole lot of code out there in an unexpected and hard to notice way. Even the complete removal of fs.existsSync would do much less harm than what you are proposing.

[matthew-dean]

@ChALkeR Possibly. It's not really want I want, just another idea. Probably the more compatible with existing code would be to return a falsy value on an error.

[jnordberg]

I would argue that changing existsSync to throw errors that where previously swallowed is not a big change, more like a bug fix. From the documentation I actually thought this was the behaviour. I mean who would think that a function that checks if a file exists would say that the file does not exist if it encounters an error?

The asynchronous version I think it should be kept deprecated, there's no way to change it without silently breaking a lot of peoples code.

[ChALkeR]

I would argue that changing existsSync to throw errors that where previously swallowed is not a big change, more like a bug fix.

That would also break a lot of code, as I assume.

[evanlucas]

@jnordberg changing existsSync to throw where errors were swallowed before would break a ton of code.

[jnordberg]

I'm trying to think of scenarios where code would break and i'm drawing a blank. Could you give some example?

[evanlucas]

https://github.com/search?l=javascript&p=96&q=existsSync&ref=searchresults&type=Code&utf8=%E2%9C%93 shows a ton of repositories that use it. How many of those would actually break from that change? I'm not sure. But, this would be a semver-major change

[jnordberg]

Heh, removing the function would break all of them for sure ;)

[dfabulich]

It seems like there are really two criticisms of exists/existsSync. One criticism was that exists doesn't use nodeback, which is a non-issue for existsSync.

But the other problem seems to be the argument that exists/existsSync promises too much certainty. You can know that a file definitely does exist, but if you can't access it, there's no way to know whether that's because the file truly doesn't exist or because of some other problem (security, connection issues, etc.). access/accessSync fixes the semantics.

I think that argument is excessively cautious. No other language runtime I've ever used has bothered to protect me from those edge cases. Java, C#, Python, PHP, Ruby, Perl and Swift have all ignored those problems and offer a simple boolean "exists" test; I think that's the correct decision here, too.

So existsSync, in particular, should be undeprecated, with warnings in its documentation that it's really a wrapper around statSync and so it can return the wrong answer in rare cases.

But if you insist on defending the user from these very rare and unimportant edge cases, I guess as a script author I just want a boolean, and I don't really care what the function is called.

accessSync now has painful semantics, requiring me to wrap it in a try/catch block. It's pointless boilerplate for simple scripts.

So, how about an isAccessibleSync method? It would be available only in synchronous mode, and its implementation would be trivial:

fs.isAccessibleSync = function(path, mode) {
  try {
    this.accessSync(path, mode);
    return true;
  } catch (e) {
    return false;
  }
};

You might argue that this function is too trivial to be of use, but I bet that without this function in the standard library, it will be written and rewritten and rewritten thousands of times.

It would be even better to undeprecate existsSync, but if that's just not politically feasible, at least throw us a bone and give us _some_ synchronous function that returns a boolean.

[matthew-dean]

I think that argument is excessively cautious. No other language runtime I've ever used has bothered to protect me from those edge cases.

1,000x this.

So, how about an isAccessibleSync method?

I would think accessibleSync is more "node-like", but yes, this too. If the argument is that "exists" sends the wrong message about a file actually existing (which I agree is a weak argument for removal), then maybe just renaming to reflect what's really happening is good. Although: expect people to turn around and ask for how they just tell if a file exists.

Or: just un-deprecate existsSync. Note the caveats in the docs about false positives. Don't die on this hill. I can't see it being a big deal to the Node team to just un-deprecate it, whereas it is demonstrably a big deal to a number of developers to have it removed. Whether you individually agree that it should be a big deal to developers, or should _not_ be a big deal to use another method is irrelevant. It's of utility as is.

[Fishrock123]

Note: for the case where existsSync() fails (I.e. EPERM on Windows) your code will fail (strangely) anyways, so throwing isn't actually that crazy of an idea.

On Nov 23, 2015, at 8:25 PM, Matthew Dean [email protected] wrote:

I think that argument is excessively cautious. No other language runtime I've ever used has bothered to protect me from those edge cases.

1,000x this.

So, how about an isAccessibleSync method?

I would think accessibleSync is more "node-like", but yes, this too. If the argument is that "exists" sends the wrong message about a file actually existing (which I agree is a weak argument for removal), then maybe just renaming to reflect what's really happening is good. Although: expect people to turn around and ask for how they just tell if a file exists.

Or: just undeprecate existsSync. Note the caveats in the docs about false positives. Don't die on this hill. I can't see it being a big deal to the Node team to just un-deprecate it, whereas it is demonstrably a big deal to a number of developers to have it removed. Whether you individually agree that it should be a big deal to developers, or should not be a big deal to use another method is irrelevant. It's of utility as is.

—
Reply to this email directly or view it on GitHub.

[crrobinson14]

I'd like to join the crowd of voices calling for un-deprecating this. File-exists-semaphore mechanics are a valuable and useful tool, and extremely performant in any modern OS. This whole "it's an anti-pattern, just open it" is just a silly, indefensible argument. There are just SO many valid reasons why you might want to know if a file exists without actually wanting to open it. Don't get judgmental about how other developers solve a problem. This isn't Ruby.

fs.access() does work... but THAT is the anti-pattern. Using a function to do X that was designed to do Y is what people complain about in PHP. Is that where Node is going?

[benjamingr]

Honestly I'm fine with existsSync @Fishrock123 because people would not be using it for the race condition case anyway and it does seem useful.

[bnoordhuis]

This whole "it's an anti-pattern, just open it" is just a silly, indefensible argument.

I feel the anti-pattern crowd has been defending it quite well so far, actually. :-)

There are just SO many valid reasons why you might want to know if a file exists without actually wanting to open it.

Maybe you can name a few?

[jnordberg]

@bnoordhuis if (!fs.existsSync(dir)) fs.mkdirSync(dir)?

[matthew-dean]

I feel the anti-pattern crowd has been defending it quite well so far, actually.

Where was that, exactly? You could tell if a file existed by using fs.utimes, because if it didn't, it would throw an error. You could tell if a file existed by using fs.rename, because if the file doesn't exist, it would throw an error. But it's equally arbitrary and silly to say, "If you want to check if a file exists, just attempt to rename it" as it is to say, "If you want to check if a file exists, just attempt to open it." The reason it's an anti-pattern is because none of those other file operations represent the author's intent, even though they can, as a side-effect, provide the wanted information.

Maybe you can name a few?

There are plenty of examples throughout this thread.

[crrobinson14]

Sigh. This is one of the things that makes people complain that this community can seem hard to approach. You have this big crowd of people begging to keep something that's clearly valuable to them, and it feels like just a stone wall. This entire thread is full of people listing their reasons, yet you're asking for MINE? Fine, I'll bite... but why do I have the feeling it won't make any difference and I'm wasting my time?

1. I have an application that produces a custom image with a chart of a data set. Periodically another application comes along and "picks up" (consumes and deletes) the file. This is a closed-source system that cannot be changed. Having the file disappear is the signal to create a new one.
2. I have a trading system, another closed-source app I have no control over, that reports compliance-management-shutdowns via "XYZ.down" semaphore files. My app is expected to check for the existence of such a file every second. Sounds crazy, but that's the way it works - and it works really well, actually, because the semaphores are written on a temp filesystem so they're all in-memory anyway.
3. I maintain an old upload management tool that has a security check to make sure a referenced temp file exists before calling an external processing script to convert it to a final storage format.
4. I have a tripwire security system that a customer requested to work a specific way. It alerts an operator on the disappearance of any of a set of pre-defined files.

Just to be fair, can somebody please contribute a list of applications where this deprecation improved the application?

This is how enterprise dev goes when you don't control the whole environment, and sadly that's a lot of the time. Love it or hate it, it's the real world and file-existence-tests are a super-basic and very common operation.

And yes, you can still them in Node... But the WAY you do it is UNCLEAR. I'm arguing that the new suggestions run contrary to the basic principle of writing clear, readable, and maintainable code. If you want to test for the existence of a file, you should call something that clearly does that, not something else that does it as a side effect.

There is a reason why almost every major programming and shell scripting language has a file-exists test. This isn't just syntactic sugar. Just for giggles I grepped my /sbin and /usr/sbin files and have 37 Bash scripts alone that do [ -f FILE ] tests. Are we arguing that Node is "different" somehow? That's "it's not for those kinds of uses?"

This thread alone has been running since May, and this is not the only thread where people are complaining about this policy. The entire discussion and agita in the whole community has certainly exceeded the net sum value to the development community of the "cleanup value" of deprecating this feature.

The most disappointing thing is that the only argument I've seen here that says "hey developer, here's how your life will be BETTER by this change" has to do with API consistency. That makes me sad. Deprecation is a breaking change -- why not just make it consistent, instead?

[bnoordhuis]

if (!fs.existsSync(dir)) fs.mkdirSync(dir)?

Race condition. This is exactly why I think it's good to keep it deprecated; people get it wrong more often than not.

[matthew-dean]

This is one of the things that makes people complain that this community can seem hard to approach. You have this big crowd of people begging to keep something that's clearly valuable to them, and it feels like just a stone wall.

Not only that, but just the fact of the ubiquity of such a method in other languages (and the historical existence in Node) should be enough to keep it. By removing an exists method, you're suggesting that the Node contributors know better than, what, everyone? Somehow everyone else has it wrong? What do you think is the likelihood of that? Even if you say "very likely" (which is astounding), you're banking pretty hard on that over the needs of other developers (who you seem to think "get it wrong") by removing the exists methods.

Just to be fair, can somebody please contribute a list of applications where this deprecation improved the application?

I would also like to see said examples.

[dfabulich]

I feel like there's two arguments here, one about fs.exists and one about fs.existsSync. In an effort to clear this up, I've started a new issue #4216 to discuss the synchronous version specifically.

[matthew-dean]

@dfabulich I feel like the two arguments are pragmatism vs. hubris, but that may be true. I probably only ever use existsSync.

[crrobinson14]

To clarify, existsSync is the only aspect I'm really pushing for as well. I can totally get behind the argument about exists and I personally don't use it.

However, I don't buy the argument about the race condition. There are race conditions in ALL of these methods. access and accessSync aren't immune to them - an external process could remove the file after you check it but before you do the next thing. File-existence-checking is subject to race conditions no matter HOW you do it and no matter what language you do it in - Bash, PHP, Python, whatever. That's just the nature of the beast with this kind of mechanism. It shouldn't be Node's job to "fix" it.

[bnoordhuis]

However, I don't buy the argument about the race condition. There are race conditions in ALL of these methods.

@crrobinson14 There is no race condition when you call fs.mkdirSync() unconditionally and handle the EEXIST exception when it happens.

@matthew-dean No word games, please. Professional discussion only.

[crrobinson14]

@bnoordhuis That only works if you actually WANT to create the directory. What if you just want to know it isn't there, but it's not your job to create it? Same issue as for files.

[crrobinson14]

For that matter all of the proposed solutions would have the same race condition if that's actually what you care about. access, accessSync, stat, statSync - all of those could have an external actor change the examined environment while you're getting into your next step. Nothing here says you shouldn't be try/catching your file open operation. It's just a big crowd of people saying "but I might not WANT to open the file as my next step."

[matthew-dean]

@bnoordhuis Word games? It's not a game; that's my perception.

+1 to what @crrobinson14 is saying about race conditions. Race conditions were addressed earlier. Not all "check for existence" calls are immediately followed by another file operation on that file. And the two sync functions in a row is not a race condition in terms of how it's traditionally defined; that is, two asynchronous methods in your code, where the second depends on the first. Synchronous code is not subject to race conditions. Yes, any external process can crap up some sequence of file operations, but that's not a race condition. A hard drive sector failing after you open and begin writing to it is not a race condition.

[bnoordhuis]

That only works if you actually WANT to create the directory.

@crrobinson14 Which is what the posted example wanted to do. If you're not going to do anything with the directory, why would you care if it exists or not?

access, accessSync, stat, statSync - all of those could have an external actor change the examined environment while you're getting into your next step.

@crrobinson14 If the next step is modifying the target of those system calls, then yes, you're using them wrong.

Word games? It's not a game; that's my perception.

@matthew-dean Words like "hubris" are loaded language. It has no place here.

[bjouhier]

The race condition is a red herring. Of course, there are scenarios where race conditions are possible, even likely; but there are also plenty of real-world scenarios where race conditions are impossible _by design_. One example is testing files that may or may not have been installed together with the software but that are never created nor deleted dynamically by the software itself.

We have plenty of calls to fs.exists in our application (and a few to fs.existsSync), and we've never faced any race conditions on them. Simply because the context in which they are executed is such that files don't get created or deleted concurrently (unless there is a malicious process running on the machine - in which case the whole integrity of our software is compromised anyway).

Deprecating the calls will be a problem for us. Not so much for our own code (we can easily replace by our own equivalent implementation) but for dependencies. Some of them will break and I'm not sure that all of the dependencies will get fixed and re-released in a timely manner (so we may have to fork them and publish under another name - painful). And sometimes we use some old stuff for a very precise purpose, it just works, has always worked, and we just don't want to take the risk of upgrading it: if it ain't broke don't fix it.

JavaScript managed to get very far while "not breaking the web". We still have substring and substr (with different interpretation of the 2nd arg). This will hurt the feelings of any API purist but these APIs are here to stay, whether we like it or not. Even methods like bold or italic that are non-standard will not be removed any time soon from browser engines.

[dfabulich]

Gripers may be interested in my PR #4217, which adds an accessibleSync method that behaves like existsSync used to.

[crrobinson14]

@bnoordhuis My use-case wasn't about directories, it was about files, and I provided four clear examples of real-world apps I maintain that check for but do not then open those files. I specifically stated that my next step in each did NOT involve modifying them.

@dfabulich I'm a fan of your PR #4217. Sadly, after reading more of these cross-linked threads and the positions taken there, I'm losing hope that the core Node devs will accept it.

What I've learned today is that they have a philosophy and they're sticking to it whether the community likes it or not. Fine, I can respect that - plenty of other project maintainers have done the same thing, and I "get" that it's a tough, often unrewarding job.

But let's not dance about and pretend this is being done for the betterment of the dev community. It's clearly not popular. I don't see any posters in this or other threads talking about how their lives have been improved by this deprecation. Nobody is saying "yes, good point, I'll totally change all my apps because I can see now that calling fs.access() and leveraging the side-effect behavior of fs.F_OK telling me it exists is going to make my code more readable or maintainable."

[bjouhier]

If the functions go away we can always monkey-patch the fs module and restore them. Ugly but pragmatic.

[crrobinson14]

That's my plan.

[bjouhier]

There is an opportunity for a very popular module here.

[bnoordhuis]

My use-case wasn't about directories, it was about files, and I provided four clear examples of real-world apps I maintain that check for but do not then open those files.

@crrobinson14 Sorry, I thought your comment was in the context of that snippet that @jnordberg posted. Let me address them.

Having the file disappear is the signal to create a new one.

Open the file with fs.open(filename, 'wx'), that ensures the file either gets created or the operation fails with an EEXIST error.

I have a trading system, another closed-source app I have no control over, that reports compliance-management-shutdowns via "XYZ.down" semaphore files. My app is expected to check for the existence of such a file every second.

Why not simply try opening the file? Or is the contents irrelevant and it's just about its existence as a single bit of external input?

That's a legitimate use case - in fact the only legitimate use case - but we decided it was niche enough in the face of overwhelming evidence of abuse that it was still worth deprecating it (and because you can emulate it easily enough through other fs methods.)

I maintain an old upload management tool that has a security check to make sure a referenced temp file exists before calling an external processing script to convert it to a final storage format.

Not passing judgement but it sounds like it's race-y by design.

I have a tripwire security system that a customer requested to work a specific way. It alerts an operator on the disappearance of any of a set of pre-defined files.

I would argue that fs.access() or fs.stat() are more appropriate for that because they actually tell you what changed.

[crrobinson14]

Thanks @bnoordhuis for hearing it all out. I don't agree with the decision but respect the amount of time you've put into tracking the whole discussion on your side of it.

[matthew-dean]

Words like "hubris" are loaded language.

@bnoordhuis If you say so. Hubris can mean thinking a particular way is superior. A core argument for this change is not that it's more useful (i.e. pragmatic), but that it creates "superior" code that's less "wrong". Both of those are subjective, and many of the back and forth are not that the developer's code does not function, or does not return the result the developer wants, but that it's simply not the best way, and other ways constitute abuse. This implies that the recommended newer approach is superior to the old, even if, for the individual developer, it is pragmatically more difficult in usage. Whether that superiority is due to philosophy or because it does not align with other Node values & goals is hard to discern. It doesn't logically follow, so yes, I'm probably making an assumption that personal bias must be the reason, and maybe that assumption is incorrect. Obviously, there are people here that think differently than me. Perhaps I should have said pragmatism vs. Node philosophy? Or pragmatism vs. API consistency?

If Node maintainers strongly feel that this decision is better for Node (and they clearly do), then, like @crrobinson14, that is essentially enough on which to base a decision. Most of my arguments have been against the cases that the deprecation decision is better for me as a developer, or that other methods are better to use, or "just try to open it" makes equal sense. For me, it's not so. But I'm also not everyone. It's frustrating, but I feel done arguing about it. ¯(°_o)/¯

[Fishrock123]

Can anyone provide examples of how other platforms deal with exists and access errors?

[crrobinson14]

PHP provides a pass-through to the usual FS "stat" category of calls, as well as direct checks for specific states like exists. The call returns FALSE for non-existent files and also for files not accessible due to permissions issues. Note that it can return TRUE if the file exists but isn't r/w/x by the user. It just has to be "visible" in the directory listing:
http://php.net/manual/en/function.file-exists.php

Bash provides a very large list of file test operations, mostly because it does NOT provide a "stat" call so you have to check each thing individually. There's the basic -e for exists, but there's also -f for exists-and-is-a-file, -r for exists-readable, etc.:
http://tldp.org/LDP/Bash-Beginners-Guide/html/sect_07_01.html

Both of these are sync calls, obviously. Neither throws an exception on any kind of access error - accessibility is just considered a separately testable operation. Neither notes the possible TOCTOU security implication.

Java has a bunch of libraries but the most common one (in my current experience) is java.nio.file.Files. That provides a method exists that returns a boolean existence check. The calls BEHAVES the same as the others above, but the docs have this to say:

Note that the result of this method is immediately outdated. If this method indicates the file exists then there is no guarantee that a subsequence access will succeed. Care should be taken when using this method in security sensitive applications.
http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#exists(java.nio.file.Path,%20java.nio.file.LinkOption...)

This call throws SecurityException if the file exists but is not accessible by the user. Curiously, the test used is readability. If the file is writable but not readable, it throws. It's really annoying, actually, when you're dealing with things like write-only pipes or log files, or filesystems with odd behaviors. I've run into those before.

Python provides the curiously named os.path.exists which does what it sounds like but is mostly used for files, not paths. Python is even more annoying than Java for cross-platform devs - as noted in the docs On some platforms, this function may return False if permission is not granted to execute os.stat() on the requested file, even if the path physically exists. It behaves differently on Windows and Unix (and this is also filesystem-dependent). No exceptions are thrown.
https://docs.python.org/2/library/os.path.html?highlight=exists#os.path.exists

Ruby provides file.exist? as a replacement for a recently deprecated file.exists?. This one has a twist. It says “file exists” means that stat() or fstat() system call is successful. That's a pretty explicit description of what's actually happening underneath it - I kind of like that approach for documentation. It forces devs to take more accountability for what's happening underneath and feels like a healthy thing to do. Ruby has a separate check for directories - the others above do not.
http://ruby-doc.org/core-2.2.0/File.html#method-c-exists-3F

C# provides system.io.File.Exists and also distinguishes between files and dirs. Its behavior more or less matches the others except that it also checks for invalid strings along the way and specifically notes in the docs a variety of combinations that will also return False. It even references a separate function for checking for invalid path characters. C#'s docs also specifically mention the security implications, saying Be aware that another process can potentially do something with the file in between the time you call the Exists method and perform another operation on the file, such as Delete.
https://msdn.microsoft.com/en-us/library/system.io.file.exists(v=vs.110).aspx

Erlang doesn't provide a "pure" exists check, but it provides is_file. This is a combination test for file existence and type (non-dir, non-symlink). The other platforms all required different operations to do dir/symlink/file checking. No access check is performed.
http://erlang.org/doc/man/filelib.html#is_file-1

[geoffp]

My use case is that I need to rename a file from path A to path B, but only if a file at path B doesn't exist already. If anybody knows of a more straightforward way to write that than a simple existence check, I'd be very interested to know what it is.

I mean, I'll try fs.access() or fs.stat(), but that doesn't read as expressively as fs.exists() for the next person who has to maintain my code.

[bnoordhuis]

@geoffp When you say "a file" do you mean a file of the same name at location B? In that scenario you would call fs.rename() and be prepared for EEXIST errors.

If it's a file with a different name, there is no easy way to do that free of races. (There are complicated ways to do it free of races but none are very portable.)

[crrobinson14]

I just got a new one yesterday doing almost the exact same thing. I have a service that can never NOT have a crossdomain.xml file in a given hosting folder. When the server starts it checks to see if it exists. If it does, it leaves it alone - the admin may have created a custom one. If not, it copies a template file there so at least a default can be served.

Same story, though. Just doing a copy/catch is wrong because you don't want to copy unless it does NOT already exist. accessibleSync is ugly because the server doesn't care about accessibility. I went with stat for now but it ended up being several lines of code where one would have done.

I figured out a good way around this though. I just duck-punched fs to make an existsSync that calls statSync and returns an appropriate value. I'm sure this will emerge as the standard answer on thousands of StackOverflow "how do I check if a file exists?" questions over the next few years. It's a shame core NodeJS can't do something so basic, but hardly the end of the world...

[benjamingr]

@crrobinson14

Just doing a copy/catch is wrong because you don't want to copy unless it does NOT already exist.

I assume you do something like:

fs.createReadStream('defaultCrossdomain.xml').pipe(fs.createWriteStream('crossdomain.xml'));

fs.createWriteStream accepts a second argument for options where you can specify to open the write stream only if there is no file there letting you avoid the race condition.

[crrobinson14]

@benjamingr No, I moved this to a Bash script which has a proper -f test and can do it in a single line of clearly-readable code. I was only sharing what I WOULD do with others, if I wanted to keep this in NodeJS. Given recent changes in Node's direction, I've shifted my own personal strategy to use it mostly for API services that don't need to do much file work. Every language has its place, and Node is great for that purpose.

I appreciate your suggestion, but the thought of opening and reading an original/template file, then piping it to a writer whose purpose is to fail if the write was unnecessary is so mind-bogglingly unclear and inefficient that I'd never do it that way even if it was the ONLY way. I'd rather exec to a script that can do the job properly.

[benjamingr]

@crrobinson14 I'm not sure what the "no" is for - I just pointed out that it's entirely possible to do this in Node without the race conditions associated with using exists, and that the race conditions would be there if you use exists anyway.

[crrobinson14]

See, this is where this whole conversation gets off track. Node can't protect developers from race conditions forever - it's so easy to create them unintentionally that to save people from THIS ONE is just silly. There is no race condition in MY application because the file cannot be served OR created while the server is starting. You're addressing a concern I never had and never mentioned myself. So you're solving the NodeJS core developers' concern, not my concern.

That's why I turned down the option. To me, it's even less clear than accessibleSync. Clearly understandable code is a critical priority for me - I write a lot of code that others have to maintain in the future. None of these options address that need without heavy commenting saying "I'm doing all this stuff because they deprecated the 1-liner I used to use that did exactly what all this stuff pretends to do."

[geoffp]

@bnoordhuis correct, a file of the same name. That approach actually sounds very elegant, but I don't think I could have learned that from the documentation -- I didn't see any mention of whether the default behavior of fs.rename() in this case is to overwrite an existing file or throw an error. It wasn't obvious from reading the NodeJS code or posix rename documentation, either.

EDIT:
Doesn't seem to work. The default behavior of rename(2) is to overwrite. I'm back to a manual existence check with fs.access().

[trevnorris]

@crrobinson14 See my comment above how using access()'s F_OK is equivalent of checking whether the file exists or not. Only time that isn't strictly the case is on Windows. In which case there are other issues.

So accessibleSync() would function, essentially, the same as existsSync() except it doesn't suffer from the Windows permission issue, and it's faster.

[crrobinson14]

@trevnorris That's why my long-term solution will be an accessibleSync wrapper called existsSync. As I noted above, my primary complaint here is about code clarity. When you're concerned about security and reliability, clarity is a crucial detail because future maintainers of a code base need to perfectly understand what a piece of code is doing.

My argument is that the following is unclear:

fs.F_OK - File is visible to the calling process. This is useful for determining
if a file exists, but says nothing about rwx permissions. Default if no mode
is specified.

Using side-effect-behavior to determine developer intent is a bigger invitation for problems than the other possible issues I've seen mentioned so far. I don't even know what F_OK is supposed to be telling me here - existence without accessibility means "OK"?

I can see (not agree with, but see the rationale behind) the argument that there is a possible TOCTOU pathway behind wanting to do an exists-check in the first place, separate from actually using the asset. What I fail to see is how this can be considered ok:

// Check to see if the file exists using a side-effect of accessSync - don't worry,
// we don't actually care if we can access the file. It's just that accessSync is the
// only function that can do this because existsSync got removed... Don't even
// get me started about how this is gated off fs.F_OK...
if (fs.accessSync(..., fs.F_OK)) {

but this cannot:

if (fs.existsSync(...) {

The latter requires no comment because it's clear what's happening. If, in the end, fs.existsSync = fs.accessSync.bind(fs) are functional equivalents why not just end the whole argument by providing the former as an alias for the latter and documenting that this is what it's doing? After all, appendFile is more or less an alias of writeFile with a flag of 'a'. Why not provide the same nicety here and eliminate the dozens of StackOverflow questions that will surely come out of this?

[dfabulich]

Just to be clear, both the accessibleSync() PR #4217 and the "undeprecate existsSync" PR #4077 are now closed. Unless one of those PRs is reopened, this situation will never improve.

1) @benjamingr is incorrect that createWriteStream allows you to open the stream only if the file doesn't exist. That's not an actual option. (Or, if it is, it's totally undocumented and not obviously visible in the code.)

2) @bnoordhuis is incorrect that rename throws EEXIST errors. rename replaces the destination file. And this can never be fixed. All node could do is pre-check for the existence of the file, re-introducing the race.

3) As far as I know, no one has ever successfully argued against using existsSync for inter-process communication (IPC). For example, in PR #4217 I gave the example of checking for the existence of .git/MERGE_HEAD in a git hook, to detect whether a merge is currently in progress. Fundamentally, the filesystem is a way for processes to communicate with each other, sometimes by checking for the existence of files.

The right thing to do here is to reopen #4077 and merge it.

[bjouhier]

This TOCTOU discussion reminds me of a terrible design decision that was taken in the early days of Java. It can be summarized as:

Let us mark all collection methods as synchronized. This will prevent developers from shooting themselves in the foot.

This was terrible for two reasons:

• performance: every call had the overhead of a synchronization
• false sense of safety: it lured developers into thinking that their code was thread-safe when it was not. Individual calls were safe but combinations of calls were very often still subject to race conditions => developers had to add another layer of synchronization in their code anyway.

This design flaw was corrected in later versions of the JDK. The new collection classes are unsynchronized by default (but you can still get a synchronized variant if you want to).

The same holds for file system operations (the false sense of safety, not the performance issue): atomic operations are free from race conditions but operations that combine several atomic operations are usually not. To be on the safe side, you need a higher level mechanism (some kind of lock somewhere, a lock-free algorithm, a switch to _exclusive_ mode, ...).

So, deprecating fs.exists is very much like deprecating list.isEmpty. Of course, these things are inherently unsafe in concurrent contexts. But they are perfectly legit (and expressive) in contexts where concurrency conditions have been eliminated by some other means.

[trevnorris]

I'm throwing this onto the CTC meeting agenda and will be taking all the feedback given here for further discussion.

Re-opening so I don't forget.

[crrobinson14]

:+1:

[AkashaThorne]

[Fishrock123]

Python provides the curiously named os.path.exists which does what it sounds like but is mostly used for files, not paths. Python is even more annoying than Java for cross-platform devs - as noted in the docs On some platforms, this function may return False if permission is not granted to execute os.stat() on the requested file, even if the path physically exists. It behaves differently on Windows and Unix (and this is also filesystem-dependent). No exceptions are thrown.
https://docs.python.org/2/library/os.path.html?highlight=exists#os.path.exists

@crrobinson14 That sounds like our current existsSync(). I.e. It becomes ambiguous if the file exists but is not accessible. There is no good way around that, so our logical route was to suggest accessibleSync().

Ideally, fs.existsSync() would just use access, but that's not possible for reasons now possibly lost to time as noted in https://github.com/nodejs/node/pull/4217#issuecomment-168811245. (If you are super concerned about this, you are welcome to try and dig those conversations up, but I don't think it is an effective use of our time. :/)

The latter requires no comment because it's clear what's happening. If, in the end, fs.existsSync = fs.accessSync.bind(fs) are functional equivalents why not just end the whole argument by providing the former as an alias for the latter and documenting that this is what it's doing?

See above. :/

---

My use case is that I need to rename a file from path A to path B, but only if a file at path B doesn't exist already. If anybody knows of a more straightforward way to write that than a simple existence check, I'd be very interested to know what it is.

As far as I can tell, this isn't solvable without an access or stat check since rename(2) overrides by default and doesn't have an option not to.

[Fishrock123]

Just to be clear, both the accessibleSync() PR #4217 and the "undeprecate existsSync" PR #4077 are now closed.

https://github.com/nodejs/node/pull/4217 was never closed and is probably the most viable option, although I'm sure not everyone will be 100% happy with it we need to work with what we can. (i.e. we can't just go changing (breaking) fs.existsSync()'s semantics.)

[jasnell]

This issue was discussed on the CTC call today. We weren't able to come to a complete consensus on it but it was decided to take another look at the deprecation strategy and see if we can refine how deprecation is done. We'll also discussed the various issues specifically around existSync and agree that a better solution needs to be developed than simply saying "don't do this". Since we couldn't come to a resolution on the call today, however, we decided to punt the conversation back to github to see if we can come to a resolution.

[bnoordhuis]

As far as I can tell, this isn't solvable without an access or stat check since rename(2) overrides by default and doesn't have an option not to.

Sorry, missed the comments about rename(2). @dfabulich is right that it overwrites the target but you can work around that by hard-linking the old path to the new path (which fails with EEXIST if a file with that name already exists), then unlinking the old path. I.e.:

try {
  fs.linkSync(oldPath, newPath);
  fs.unlink(oldPath);
} catch (e) {
  if (e.code == 'EEXIST') {
    // handle error
  }
}

Caveat emptor: not sound on NFS mounts but then neither is rename().

[dfabulich]

@bnoordhuis That just moves the race condition around. Something could happen to the _old_ path while you were creating the hard link, causing the unlink call to fail (or worse, delete something that you didn't mean to delete).

[bnoordhuis]

Sure, but the original question was how to rename only if the target doesn't exist, no constraints on the source. The code above is sound as long as all actors use atomic create-or-fail ownership logic for source and target.

[dfabulich]

Couldn't somebody successfully unlink the old file and replace it with something important between the link/unlink in your example?

[Fishrock123]

@dfabulich Yes, but the same thing is also possible with exists*.

[DrunkenBeetle]

Sorry if this is repeating but this thread is way too long for me to read fully :)

I do use (well use to) the fs.existsSync function for synchronously checking AND synchronously reading config files on app start-up, and in cron files if bash isn't playing right. Here are my 2 cents worth on this, which is probably worth 1 cent considering I didn't read the entire thread:

• Dumping this but also providing a replacement is fine, but replacing a returned boolean false with a thrown error isn't. If fs.accessSync() and fs.statSync() returned false then the below code would work with either function being used:

if (!fs.existsSync('/path/to/nowhere') && !fs.accessSync('/path/to/nowhere') && !fs.statSync('/path/to/nowhere') {
  console.log('It REALLY doesn\'t exist');
}

• In my case I use fs.existsSync() to check for a config file and then read in synchronously. People have complained that _someone_ could change/delete/whatever the config file after the sync check and before the sync read. Granted yes that could be a possibility, but if you are that worried about it then slap some file permissions on your app or secure up your box and problem solved, which honestly you should be doing anyway (trust no-one).

Having said all that, I can get around it with a try/catch or use a module like path-exists (so many modules now), so life isn't too bad, but seeing that deprecated flag on the docs was like waking up and stubbing your toe on a cold morning. Annoying but you'll live

PS: I'm not suggesting that fs.accessSync() and fs.statSync() be changed, otherwise there'll be another thread :)

[rvagg]

removed ctc-agenda label, it can go back on if this doesn't get anywhere and we need to try for a circuit-breaker again

[matthiasg]

we where just bitten by the different problem that fs.existsSync returns false for an existing file when hardlinks are used in-between. that is something that i find much more serious than a non-standard syntax.

[perrin4869]

Here is a use-case where a simple boolean return value makes sense and checking for errors does not:

Promise.join(fs.access('path'), somethingAsync(), anotherAsync())
.then(...)
.catch(...);

In that example using bluebird, if path doesn't exist, an error gets thrown and the other then clause never gets executed, instead you get catch. In the case where race-conditions are not a problem (in my case I just check for a stylesheet existence, and include it if it exists) I see no reason why this shouldn't be allowed. A simple warning in the documentation about not using fs.exists before reading a file should suffice.

Edit: The stylesheet in question is created by a Makefile at build time

[benjamingr]

You're using Promise.join wrong and could use .reflect to solve your gripe.

[perrin4869]

How is it a wrong usage? There are a bunch of values from database, filesystem, etc, that I'm waiting for before rendering, seems totally sensible to me.
Also, reflect doesn't completely solve the issue, because what if there is a genuine error (i.e., not a "file doesn't exist" ENOENT error), that gets thrown? In that case you do want the catch clause to be executed, but because of reflect it won't.

[benjamingr]

No, I mean literally you shouldn't use join without a callback - that only works for back-compat look at the docs - you can use .all.

You can reflect and inspect the value or .catch that promise directly and use Petka's error predicates helper to ignore enoent - should be simple enough.

[perrin4869]

Oh damn, didn't notice that specific API change, guess I'll update later.
In any case, sure, you can write a wrapper like the one by mscdex all the way up the thread, or manually inspect the value with reflect no problem, but still, semantically speaking wouldn't it make the most sense to just return true or false in this case? All I'm trying to point out is that there are genuine use cases for an async exists method, the one above seems one of them, anything else feels like an unnecessary hack

[benjamingr]

Also in your code it wouldn't work since you'd have to promisifyAll fs first and then use accessAsync. That would have been broken for fs.exists anyway because it never took a node-back style callback.

Let's focus on your case though - what if the build creates or removes the stylesheet after you checked for it and before you included it? Wouldn't it be better to try and include the stylesheet _anyway_ and handle failures there?

[perrin4869]

Hm... breaking backwards compatibility here with exists and making it a node-back style callback would make sense, wouldn't it? Especially since it has been deprecated for a long time. Otherwise, the proposed fs.accessible method above could be node-back style callback.

In my specific case, the make command would supposedly run at build time, before the server is started, and if it is ever run again, it would be after stopping the server and running make clean. So if this workflow is followed, your hypothetical case would never occur. Including the stylesheet anyways would result it an unnecessary 404. Specifically, I want to include language-specific stylesheet in an i18n environment, but not all languages have special styles.
Is there something I'm missing then?

[benjamingr]

Hm... breaking backwards compatibility here with exists and making it a node-back style callback would make sense, wouldn't it?

That would break _a lot_ of existing code, just because we put a nice "deprecated" sticker on exists doesn't mean people aren't still using it. It would break lots of code.

[perrin4869]

Yeah I don't know any methodologies for these kind of settings... But if it's deprecated for long enough, you can eventually remove it or change right?

[imyller]

@benjamingr @perrin4869
Why not just make it work with both worlds.

This solution works fine if called with callback(exists) and callback(err, exists).

Legacy code expecting boolean argument callback will not break and code calling it in nodeback style would also get the expected result.

This works by assuming that nodeback style has more than one function argument - as it always has - and legacy style has only one.

Obviously all the error details from fs.stat are lost, but isn't that the point of fs.exists?

fs.exists = function (path, callback) {
    if (!nullCheck(path, cb)) return;
    var req = new FSReqWrap();
    req.oncomplete = cb;
    binding.stat(pathModule._makeLong(path), req);
    function cb(err, stats) {
        if (callback) {
            if (callback.length > 1)
                callback(null, err ? false : true);
            else
                callback(err ? false : true);
        }
    }
};

[perrin4869]

@imyller Cool! Errors details are indeed irrelevant :)

btw, just as a sidenote, I was curious so I checked the following: I just went over my project's node_modules directory, installed with npm@3 and [email protected] it has 911 different repositories. I ran the following commands:

$ ag 'fs.exists' -l | wc -l                                                                                                                                          
133
$ ag 'fs.exists\(' -l | wc -l                                                                                                                                        
29
$ ag 'fs.exists\(' -l                                                                                                                                                
write/index.js
pmx/lib/probes/profiling.js
gulp-symlink/index.js
coffee-script/lib/coffee-script/command.js
node-sass/scripts/build.js
fs-extra/lib/json/output-json.js
fs-extra/lib/ensure/file.js
fs-extra/lib/ensure/symlink.js
fs-extra/lib/ensure/link.js
fs-extra/lib/copy/copy.js
fs-extra/lib/output/index.js
fs-extra/lib/ensure/symlink-paths.js
find-up/node_modules/path-exists/readme.md
jspm/lib/install.js
jspm/lib/package.js
mkdirp/test/opts_fs.js
mkdirp/test/opts_fs_sync.js
jspm-npm/lib/node-conversion.js
phantomjs/lib/phantom/examples/scandir.js
phantomjs/lib/phantom/examples/scandir.coffee
express-winston/node_modules/winston/lib/winston/transports/file.js
systemjs-builder/lib/trace.js
vizion/lib/identify.js
winston/lib/winston/transports/file.js
path-exists/readme.md
jspm-github/github.js
rmdir/Readme.md
rmdir/lib/rmdir.js
pm2/lib/tools/VersionManagement.js

As you can see, most of the calls are actually to fs.existsSync, with only 29 references to fs.exists. Not terribly significant, is it?

[crrobinson14]

Ummm...

That would break a lot of existing code, just because we put a nice "deprecated" sticker on exists doesn't mean people aren't still using it. It would break lots of code.

But... you're removing it entirely... How is that better?

[benjamingr]

@crrobinson14 no one is removing anything entirely from anywhere. fs.exists will continue to exist in node for all eternity - there is no way to remove it realistically in the next 5-10 years without breaking the ecosystem.

[crrobinson14]

@benjamingr This whole thread and related uproar came up because of the deprecation of fs.exists and fs.existsSync. Both are marked deprecated.

[bnoordhuis]

Deprecation != removal.

[felixsanz]

fs.access is "Tests a user's permissions for the file specified by path"

I want to check if the file exists, not if the user have permissions for the file.

Reason: A blog that uses file storage using "post_title.md" as filenames. When i create a new post using a CLI tool, first i want to check if that title is already used just by checking if the file exists.

No race conditions, no need to open the file or check if the user have permissions for that.

Please bring fs.exists back.

[bnoordhuis]

@felixsanz What's wrong with fs.open(filename, 'wx') and handling the EEXIST error? If your argument is "it's more work", my counterargument is "it's a hell of a lot more robust."

[felixsanz]

@bnoordhuis Because fs.open is self-explanatory for "open a file", and... i just don't want to open it. Not even by far.

Check existance !== open

You know, when you browser the docs for your "exists" method, you will never think that "fs.open" is the one you need. Because of that, it will confuse new developers for example. Not having an exists method is, in my opinion, a hole in the FS API. If it was broken, just fix it. Don't workaround it with methods that have nothing to do like "fs.open" (open a file) or "fs.access" (check for user permissions).

Sure, you can accomplish the same functionality but that's not the way it's mean to be. You can also use Strings for dates, but that's why the Date object exists. Don't mock functionality...

[imyller]

Only drawback I see from implementing my proposed solution is that it might promote the use of already deprecated fs.exists. "incompatible" callback function parameters were not the only reason for deprecating fs.exists; there are also internal implementation issues related to filesystems and platform portability.

However, I'm happy to create a PR for this feature if there is enough support and it is seen desirable to add backwards compatible functionality to already deprecated function.

[GeorgeBailey]

@imyller, I would much prefer that folks just use your code where they want it, and not to include this as part of Node.JS core.

[imyller]

@GeorgeBailey That was a minor modification to current fs.exists function from lib/fs.js in Node.js core. As is, it's not usable as a standalone solution as it refers to private helper functions inside the fs library.

[GeorgeBailey]

[merged in earlier comment] I've experienced where having individual functions (like Java had separate functions for isFile, exists, length, etc.) disguises the performance hit from a novice developer. I can see someone calling both exists and stat sequentially, which would be a waste.

Also I really like keeping Node.JS core plain and simple. Consistency is an important part of this.

When i create a new post using a CLI tool, first i want to check if that title is already used just by checking if the file exists.

@felixsanz, For your use case, you should use stat not open.

You know, when you browser the docs for your "exists" method, you will never think that "fs.open" is the one you need. Because of that, it will confuse new developers for example. Not having an exists method is, in my opinion, a hole in the FS API.

So yea, you wouldn't use open, but you would use stat instead. I see what you mean that folks might not know to use this, but keep in mind that exists could be considered a convenience method because it ends up doing a stat anyway. (this is true of all programming languages) So I would suggest folks use stat over exists so that they know what they are doing, and understand that the performance impact is the same. That actually goes for file size, isDirectory, etc as well.

A simple search of the internet will tell you that stat is the answer for all of these.

If you want the convenience back, you can create a convenience method.

If @imyller's solution is implemented in Node.JS core, it does give you what you are looking for, but it makes it that much more difficult to phase-out the inconsistency that prompted it to be deprecated in the first place.

[perrin4869]

@GeorgeBailey so people should reimplement a convenience fs.exists every single time in their projects? It seems to me like a basic enough method not to have it reimplemented every time

[felixsanz]

@GeorgeBailey

If you want the convenience back, you can create a convenience method.

We could also remove console.log from the API, if someone needs it you can implement this:

console.log = function (x) {
  process.stdout.write(x + '\n');
};

And if you don't want to implement it, use a NPM library!

Sorry for the irony, but it was an example.

[imyller]

I've created a PR #5098 for my backward compatible solution to allow nodeback-style callback with fs.exists.

Obviously this does not solve any other filesystem or platform related issues with fs.exists, but fixes the unconventional callback problem.

[GeorgeBailey]

@felixsanz, I don't appreciate that example. console.log does not suffer from similar inconsistencies that prompted fs.exists to be deprecated. (you do know there is an inconsistency, right?) Also console.log includes additional features using util.inspect, so it would be more difficult to create a convenience method for it.

[GeorgeBailey]

fs.exists is an easy problem. Just for reference, let's look at the difference in code.

fs.exists(filename, function (exists) { // old format
    // when there is an error (i.e. no permission), it just sets exists to false
    var existing = exists
})
fs.exists(filename, function (err, exists) { // new format
    if(err)
        // an error
    else
        var existing = exists
})
fs.stat(filename, function (err, stats) {
    if(err && err.code != 'ENOENT')
        // an error
    else
        var existing = !!stats
})

So on the one hand, the difference in code needed is small, and the ability to implement compatibility both ways is already in @imyller's pull request. We can be happy no matter which way it goes.

fs.existsSync is more difficult problem.

var existing = fs.existsSync(filename) // old format
// when there is an error (i.e. no permission), it just sets returns false

var existing = fs.existsSync(filename) // new format
// may throw an error

try {
    var existing = !!fs.statSync(filename)
} catch (e) {
    if(e.code != 'ENOENT') throw err
}

So I can see why folks would not want to use statSync because catch becomes necessary even for normal operation. But if we are going to continue with the use of existsSync, then there is no way to support both formats. Either we introduce the new format (to be consistent with the rest of fs api), which is a breaking change, or we continue with the old format (already in place) indefinitely.

@perrin4869, @felixsanz, What do you think we should do?

Personally, I'm open to the idea of setting up fs.exists to work in the new format, but I think that fs.existsSync needs to be phased out completely before it is introduced in the new format. I realize that would take several years/ probably 2 or 3 major versions. But I don't like the alternative of just keeping existsSync around in the old format forever because it returns false when it should be throwing an error.

[GeorgeBailey]

Looking at the PR notes, @imyller's solution has some unintended consequences, also it doesn't handle existsSync. I think that we are left with only two choices

1. Leave fs.exists and fs.existsSync alone indefinitely. (inconsistent with the rest of the fs api)
2. Phase it out over time. (deprecation notice in next major version (or two), followed by complete removal for at least one whole major version)

I'm for number 2, and then we can bring it back correctly if there is still a desire for it.

[dfabulich]

@GeorgeBailey Are you caught up on my PR #4217? I argue there that the synchronous no-errors format is much more usable.

fs.accessSync throws errors the way you seem to think fs.existsSync should, and that makes it really painful to work with. It's impossible to write a simple if statement, e.g. if (fs.accessSync(file)) because if the file doesn't exist, it throws, so you have to catch the exception (and then, most likely, ignore it).

I think fs.existsSync should be undeprecated, and that fs.exists is the only function with a real problem here, and was correctly deprecated in favor of fs.access, which uses nodeback, is just as easy to use as asynchronous fs.exists, and is a teensy bit faster than fs.stat.

[imyller]

@GeorgeBailey

Looking at the PR notes, @imyller's solution has some unintended consequences, also it doesn't handle existsSync

The PR #5098 obviously does not address existSync because the PR is about adding nodeback-style callback support to asynchronous exists. That kind of change does not apply to existSync in any way - and it would be misleading for me to make any modifications to existSync in the same PR.

The unintended consequences you refer are true in case someone mixes up with callback arguments - that is true. I still see the situation improving with that patch. It is not perfect solution, but it mitigates the much worse situation making it at least little bit better.

[GeorgeBailey]

@dfabulich, @imyller, I would suggest that the fs...Sync functions should be mirrors of their async relatives. Node.JS was never designed to work well in Sync mode.

Knowing this, I posted a suggestion long ago, because I wanted to write code in the traditional single-threaded approach, and have a library take care of the translation. (so where normally a thread would block or sleep, this translates to callbacks under the hood.) It was suggested to look at node-fibers instead.

I never fully researched that route, but if we are looking for convenient Sync functionality, the fs api may not be the best way to do it.

[perrin4869]

@GeorgeBailey Eventually I would like to see a consistent fs.exists and to a lesser extent fs.existsSync, I understand that it cannot happen overnight. But then again, I think the community would be very fast in updating the whole ecosystem with new versions targeting people using newer versions of node. I guess the question would be about how to make the transition as painless as possible.

[rotemdan]

I have an existing file I'm opening for reading and appending, e.g.:

var fs = require("fs");

fs.open("myFile.txt", "a+", function(fd) {
    // ..
})

But I only want to allow the file to successfully open if it already exists, and throw an error otherwise.

I haven't found any way to achieve this without some form of separate existence checking. There is no flag that corresponds to this requirement:

(from the node documentation):

• 'w' - Open file for writing. The file is created (if it does not exist) or truncated (if it exists).
• 'wx' - Like 'w' but fails if path exists.
• 'w+' - Open file for reading and writing. The file is created (if it does not exist) or truncated (if it exists).
• 'wx+' - Like 'w+' but fails if path exists.
• 'a' - Open file for appending. The file is created if it does not exist.
• 'ax' - Like 'a' but fails if path exists.
• 'a+' - Open file for reading and appending. The file is created if it does not exist.
• 'ax+' - Like 'a+' but fails if path exists.

Changing to ax+ instead of a+ would cause open to error if the file _exists_ but that's the opposite of what is needed here. Is there any way to error if it _doesn't_ exist?

(My current workaround is to use r+ but that means I have to check the current size of the file before every write operation. This creates another problem: now every write operation _may be vulnerable to a race condition by itself_)

[bnoordhuis]

@rotemdan

const { O_APPEND, O_WRONLY } = require('constants');
fs.open(filename, O_APPEND | O_WRONLY, callback);

[rotemdan]

@bnoordhuis

Thanks. I noticed these flags in the linux man page, but they don't seem to be documented officially in node.

Question: is this guaranteed to work on windows as well? and in general is it truly cross-platform? (I'm guessing the relevant libuv source code for windows is here).

[bnoordhuis]

Not all flags work on Windows (e.g. O_NOATIME) but most of them do.

It's an undocumented but pretty stable feature. The mode strings themselves are implemented in terms of O_flag constants, e.g., 'a' is shorthand for O_APPEND | O_CREAT | O_WRONLY.

[neuroscr]

Was just about to open an issue on this. Documentation is unclear about access being a replacement. I think the real issue here is how the not found is communicated. If access/stat are to be used for an existent check, then an exception on not found is too severe. Wouldn't returning that it's not FRWX sufficient?

[dzcpy]

fs.exists is really necessary. If the api is broken can we fix it rather than deprecate it? Or otherwise, can we provide an option for fs.access, if the second parameter mode === fs.E_OK then do not throw the error and returns the boolean?

[ChALkeR]

@andyhu Once again: fs.exists doesn't do what one might think it's doing judging by its name, and that is a problem.

Just use fs.access as a replacement, it does exactly the same for all fs.exists usecases (except for having the correct callback signature).

[trevnorris]

Seems at this point the ask is for an API that does:

function doesReallyExistSync(path) {
  try { return !fs.accessSync(path, fs.F_OK) } catch (e) { return false }
}

TBH I understand wanting such a simple API, but the problem is it's not actually that simple. Take the following example where directory b is a symlink back to a:

fs.accessSync('./a' + '/b/a'.repeat(60) + '/c', fs.F_OK)

Which will result in the following:

Error: ELOOP: too many symbolic links encountered, access './a/b/[...]/a/c
    at Error (native)
    at Object.fs.accessSync (fs.js:203:11)

Is node also supposed to add the logic of checking for ELOOP and running the path through fs.realpath() to get around this issue? If so then I'd expect node to do it's best to compensate for any platform specific issues in the API. Here things get a bit hairy, and suddenly an API at first glance was one line becomes more complex. Enough, IMO, to merit its own module (there are commonly used smaller modules out there).

[dfabulich]

I filed PR #7455 to improve the performance of existsSync beyond what's possible in pure JS (by avoiding throwing an ignored exception).

[dfabulich]

@trevnorris Your accessSync example is pretty funky! Luckily, we already know what the right thing to do with that is: existsSync returns false in that case. LGTM, so let's just undeprecate that.

[imyller]

Just for reference I looked up how other languages/runtimes do this:

Python 3 (uses stat, "if stat fails, file is assumed not to exist" -method):

# Does a path exist?
# This is false for dangling symbolic links on systems that support them.
def exists(path):
    """Test whether a path exists.  Returns False for broken symbolic links"""
    try:
        os.stat(path)
    except OSError:
        return False
    return True

Java (JDK 8) (uses stat, "if stat fails, file is assumed not to exist" -method):

JNIEXPORT jint JNICALL
Java_java_io_UnixFileSystem_getBooleanAttributes0(JNIEnv *env, jobject this,
                                                  jobject file)
{
    jint rv = 0;

    WITH_FIELD_PLATFORM_STRING(env, file, ids.path, path) {
        int mode;
        if (statMode(path, &mode)) {
            int fmt = mode & S_IFMT;
            rv = (jint) (java_io_FileSystem_BA_EXISTS
                  | ((fmt == S_IFREG) ? java_io_FileSystem_BA_REGULAR : 0)
                  | ((fmt == S_IFDIR) ? java_io_FileSystem_BA_DIRECTORY : 0));
        }
    } END_PLATFORM_STRING(env, path);
    return rv;
}

Perl (uses stat, -e (exists) op is 1/true if stat has returned any results, false otherwise)

my %op = (
    r => sub { cando($_[0], S_IRUSR, 1) },
    w => sub { cando($_[0], S_IWUSR, 1) },
    x => sub { cando($_[0], S_IXUSR, 1) },
    o => sub { $_[0][4] == $>           },

    R => sub { cando($_[0], S_IRUSR, 0) },
    W => sub { cando($_[0], S_IWUSR, 0) },
    X => sub { cando($_[0], S_IXUSR, 0) },
    O => sub { $_[0][4] == $<           },

    e => sub { 1 }     <--- exists

PHP (uses stat, "if stat fails, file is assumed not to exist" -method):
Snippet from (filestat.c):

/* {{{ proto bool file_exists(string filename)
   Returns true if filename exists */
FileFunction(PHP_FN(file_exists), FS_EXISTS)

...

    case FS_EXISTS:
        RETURN_TRUE;

Go (uses stat with additional library function to test returned error for file existence clues):

if _, err := os.Stat("./thefile.ext"); err != nil {
    if os.IsNotExist(err) {
        // file does not exist
    } else {
        // other error
    }
}

Go language approach is the most interesting: they provide standard stat and then have two separate core library convenience functions for determining if returned _error_ indicates file existence or non-existence os.IsExists(err) and os.IsNotExists(err).

With exception of Go language, most common method for implementing "file exists" test seems to be just running stat and assuming file existence if anything / no error is returned.

[trevnorris]

@dfabulich

we already know what the right thing to do with that is: existsSync returns false in that case.

Not exactly. fs.realpath() should resolve the symbolic links to a path that fs.access() can handle (there's a bug in v6 where that doesn't work, for which I'm working on). So if the user got ELOOP they could then call the operation again with fs.access(fs.realpath(path));. Simply returning false would be technically incorrect.

[imyller]

For those interested:

I've published a userland module fs-exists-nodeback

https://github.com/imyller/node-fs-exists-nodeback

When loaded the module polyfills fs.exists to support both original Node.js callback style and standard error first callback style in a backward compatible way.

This may offer solution to those having issues with the callback(boolean) not working with libraries expecting functions with nodeback standard callback.

[Fishrock123]

I think https://github.com/nodejs/node/commit/7b5ffa46fe4d2868c1662694da06eb55ec744bde should fix / address this.

It undeprecates existsSync() and keeps the inconsistent-callback exists() deprecated.