fs: un-deprecate existsSync(), throw error on EPERM

[Fishrock123]

This is my conclusion from #1592 (comment).

A significant amount of people find specifically existsSync() useful (especially in simple CLI apps) and it seems unreasonable to require a module for this. Keep exists() deprecated as it breaks our errback API convention.

This commit makes existsSync() throw on EPERM. The technical problem with existsSync() (besides a race condition if used badly) is that you cannot know if a file does or doesn't exist under Windows with improper permissions.
Throwing in that case really isn't that bad of an idea, since the regular output will probably cause errors anyways down the line, and I don't think these errors can be remedied from Node.js. Instead, error so the end user can clean up their file permissions. Catching this error in user code isn't really worthwhile due to these reasons, so suggest against it.

Discuss. cc @nodejs/collaborators

[cjihrig]

I'm in favor of keeping existsSync(). People screamed to have it deprecated, and then even more people screamed to keep it.

I'm not in favor of having it throw, because at that point you could just use fs.accessSync(). I don't really like the idea of "don't catch the error."

[Fishrock123]

I'm not in favor of having it throw, because at that point you could just use fs.accessSync().

You can't though. You can't "just use accessSync()" if the code happens to be run on windows, that's awful.

I don't really like the idea of "don't catch the error."

I think the current behavior is actually worse.

[targos]

I'm not sure I understand what is the difference with the EPERM error on Windows and EACCESS on UNIX.

[cjihrig]

Isn't one of the biggest gripes about getting rid of existsSync() that it gives them a simple Boolean response? Throwing is also a breaking change on a heavily used API. You could argue that it's a bug fix, but I think people will still yell loudly.

[Fishrock123]

Isn't one of the biggest gripes about getting rid of existsSync() that it gives them a simple Boolean response?

No, it's that the others require excess options and stuff.

Ideally, existsSync() would return true or an error, but that ship has sailed long ago.

[Fishrock123]

I'm not sure I understand what is the difference with the EPERM error on Windows and EACCESS on UNIX.

@targos hmmm, I'm not really sure? As far as I can tell you can have an EPERM both if the file does and doesn't exist, is that also the case for EACCESS? The only difference then would be I don't think you can fix an EPERM from within the node apis.

[cjihrig]

No, it's that the others require excess options and stuff.

What excessive options? fs.accessSync() defaults to checking if a file exists, and fs.statSync() only takes the path as an argument.

[targos]

@Fishrock123

Take this example:

test % ls -l .
total 4
drwxr-xr-x. 2 stropitek stropitek 4096 Nov 30 19:08 a
test % ls -l a
total 0
-rw-r--r--. 1 stropitek stropitek 0 Nov 30 19:08 b
test % node -p "require('fs').existsSync('a/b')"
true
test % chmod -x a test % node -p "require('fs').existsSync('a/b')"
false

For the last call, the a/b file still exists but i cannot know that because of the EACCESS error on a.

[Fishrock123]

Sorry, It's that for accessSync() and statSync() you need to wrap them in try/{catch|finally}.

Maybe this isn't really an issue, idk.

[ChALkeR]

Adding a thow in a place that wasn't throwing before is a semver-major change.

[ChALkeR]

@targos hmmm, I'm not really sure? As far as I can tell you can have an EPERM both if the file does and doesn't exist, is that also the case for EACCESS?

It definitely is, and not on Windows-only. There are more cases where you can't be sure if a file exists or not, the most simple of them being:

>fs.statSync('/root/x')
Error: EACCES: permissiondenied,stat'/root/x'

The above result is equivalent in case when /root/x exists and when it doesn't.

There are also other cases when you can't be sure if the file exists or not, for example remote mounts, fuse-whatever, etc.

[ChALkeR]

To correspond with it's name, the function existsSync should have been doing something like this:

fs.existsSync=function(path){nullCheck(path);// No need to catch invalid paths it if we start throwing errorstry{binding.stat(pathModule._makeLong(path));returntrue;}catch(e){if(e.code!=='ENOENT')throwe;// ENOENT — the file doesn't exist. Everything else — who knows?returnfalse;}};

with a corresponding fs.exists method that would call callback(err, result), where result is true/false/undefined (in case of an error).

But that would break existng code very badly.
Overall, I am inclined to -1 on this change.

[saghul]

FWIW, I'm -1 on the change. If there is anything we can improve in the libuv layer (we emulate access(2) on Windows after all) in order to behave more consistently with Unix, give a shout at libuv/libuv.

[jasnell]

I'm generally +-0 on this change but willing to defer to @saghul and @bnoordhuis' judgement on it.

[Fishrock123]

Is anyone positive for undeprecating it and leaving it;s functionality?

Should EACCESS or EPERM just say the file exists? Saying it doesn't actually seems less correct in most cases?

[saghul]

@Fishrock123 see the example here #4077 (comment)

There is no data to say it exists or it doesn't.

[Fishrock123]

There is no data to say it exists or it doesn't.

Which is why I suggested for it to throw, because neither answer is correct. On second thought, I guess leaving it alone otherwise is fine-ish.

[saghul]

Which is why I suggested for it to throw, because neither answer is correct. On second thought, I guess leaving it alone otherwise is fine-ish.

If existsSync is going to throw then what's the advantage over accessSync?

I don't think there is a silver bullet here. Some say that are only interested in knowing if a file exists, but we can't really give an answer to that if the containing directory doesn't have the right permissions.

[domenic]

Some say that are only interested in knowing if a file exists, but we can't really give an answer to that if the containing directory doesn't have the right permissions.

Exactly. This is why I'm in favor of the status quo, of keeping it deprecated.

If the main complaint is really about having to use try/catch, we could consider introducing a fs.canAccessSync(path[, mode]) that returns true/false.

We could also explain the above in the documentation for why it is deprecated.

[sam-github]

Please don't change the API behaviour and keep the same name. Right now, I can write portable code by looking for access, and if its not there, I can use exists. If there are two versions of exists, then I'll actually need to include node-semver, and check the node version to see what its API is, or perhaps construct some more complex code. Though, frankly, I'm sometimes just using neither (edit: neither exists or access), replacing them with stat, which works on all node versions, and my CLIs usually only want rough checks for existence, such as: use the first dir that exists of A or B to store the data files.

I'm OK with deprecating and then deleting APIs we don't like, or that don't match node convention. It makes node better in the long run. If we need something slightly different from access, because access is insufficiently like exists on some platforms, then I suggest proposing a new fs API to do that, with a new name.

[trevnorris]

I'm +1 for @domenic's API concept. More explicit and predictable. There's been a lot of argument over what "exists" means, while it is much easier to define "accessible".

[Fishrock123]

Proposal

fs.accessErrorSync(path, [mode]) -- returns undefined if there is no error (i.e. file is accessible), or an Error if there is one.

Question: Would we accept that in core?

[domenic]

Does it really solve people's problems? My understanding is they want a boolean return value.

[Fishrock123]

My understanding is they want a boolean return value.

I still don't really think that's adequate, but this can be used in a boolean check if needed, or check the error if desired.

[trevnorris]

I think the point of the access check is, is it accessible or isn't it. don't see what an error would need to convey.

[Fishrock123]

Hmmm, thinking about it again, I don't think people care that much about accessible, but more if a file exists.

I.e. Does any config exist yet, or should we make a new one? If it exists there, but isn't accessible, you can't say it doesn't exist or the program will probably break.

[bnoordhuis]

Bad example. In that case you'd open the config file with 'wx' to make sure it's newly created.

Or maybe it's a great example of why fs.existsSync() should stay deprecated. :-)

[amb26]

+1 for undeprecating existsSync. I am interested in the intention, expressed via the presence of a file in the filesystem, that it should be processed - summarised as a simple boolean that doesn't require me to treat its presence or absence as "exceptional". I think we're all familiar with the recommendation not to use exceptions for normal program control flow. Whether later FS operations on the file end up throwing, as a result of a permissions/access failure, or a race condition that removed it in the intervening time, is absolutely fine with me - that condition then is exceptional.

If exists/existsSync throws early if it can detect one of those exceptional conditions I really don't mind. Also whether it returns a genuine boolean or simply a truthy/falsy value I also really don't mind. fs.accessErrorSync solves a problem I don't have. I, and I think thousands of others, feel that there's absolutely nothing problematic with the definition of whether a file "exists" or not. I think we are perfectly happy with the fact that the mere existence of a file at a particular point in time provides no guarantees that anything can actually be done with it at a later point in time. There's no need to confuse/couple these two requirements together.

[saghul]

@amb26 repeating myself: we can't give a straight answer to whether a file exists or not if its inside a directory which we cannot enter. Example:

/root/.ssh/id_rsa (your user does not have the rights to access /root, but the file exists)

What should existsSync return? It does exist, but we cannot know that.

[amb26]

@saghul - I'm perfectly happy with an exception as the answer to that

[matthew-dean]

My understanding is they want a boolean return value.

Yes, that's correct. A thrown exception defeats the purpose.

we can't give a straight answer to whether a file exists or not if its inside a directory which we cannot enter...It does exist, but we cannot know that.

This argument has been repeatedly both stated and defeated in the referenced thread. It's an overly obsessive and fixated answer, and seems to be a base argument for the original deprecation. You are technically correct, yet these edge cases are not valid justification for throwing the baby out with the bathwater.

It's "We can't be sure of the answer, therefore we'll tell you nothing, and in fact, not allow you to ask."
vs.
"Sometimes we don't know the answer, so we can't answer true." Not being able to assert the answer to existence as true, for a boolean returning function, by definition means the return value is false.

Think about the logic you're applying to this function applied to restaurant dining.

"Do you still have the fish special?"
"I'd ask the kitchen, but it's possible that the person I ask may not know, or it may be in a location that they don't know to look in. So, I can't tell you because there's an off-chance I could be wrong. Why don't you try ordering the fish and see what happens?"

The fact that the fish special cannot be PROVEN to not exist does not matter. Refusing to say yes or no based on current available data is not MORE useful than returning boolean true / false. No scientist says that they can't reach a conclusion since it's technically true that cause and effect can never be proven, or it's possible that the entire world is a simulation in the scientist's mind, therefore they should assert nothing. It's technically TRUE, but that doesn't mean that technical trueness is always deterministic as to what information is USEFUL, or if those technically true things are really all that relevant.

That said, in the world of JavaScript, were it the case that file access is the issue (if that can be determined), then it seems perfectly legitimate, as someone else suggested, to return a "falsy" value such as undefined, such that a boolean test will still tell you that the file cannot be found. And, in the event that someone needs to care about the difference between "exists" and "not found", there are other functions that CAN throw error values which can be used to split out different behavior. fs.exists and fs.existsSync are not that function. A boolean return, by its nature, is a simplification of what information could be returned, but isn't, because the developer only wants to do a simplified test.

[crrobinson14]

"Do you still have the fish special?"
"I'd ask the kitchen, but it's possible that the person I ask may not know, or it may be in a location that they don't know to look in. So, I can't tell you because there's an off-chance I could be wrong. Why don't you try ordering the fish and see what happens?"

❤️

I could not agree more - this is a perfect analogy. I guess if I was going to recap my arguments from #1592 without ranting on and on, it would be just to say "it's not Node's job to fix this particular problem, either for operating system devs or application devs." The only issue the philosophical position resolves (the race condition) is rare, not generally a problem with the use-cases described at length, and present in every other language that provides the same metaphors. Document it and move on.

[dfabulich]

@domenic wrote:

If the main complaint is really about having to use try/catch, we could consider introducing a fs.canAccessSync(path[, mode]) that returns true/false.

See pull request #4217 which adds an accessibleSync() method that returns a boolean.

@domenic, if you really think that's a good idea, please merge it. But I think we shouldn't merge it; we should merge this PR #4077 instead.

Everything wrong with #4077 is also wrong with #4217 as well. Race conditions? Check. Potential for misuse? Check. Easy to use? Check.

But #4217 clutters the API with a function that's redundant with existsSync. #4077 undeprecates existsSync, a function that has existed for years. For this reason, we should merge #4077 and close #4217 without merging it.

[trevnorris]

@dfabulich The argument for adding a function like accessibleSync() is because there's no ambiguity. While existsSync() may not seem ambiguous at first, we realize that the "existence" of a file can become convoluted. In this PR you can see that there's a specific case on Windows where existsSync() can throw because of permission (i.e. accessibility) issues. Which defeats the point of using existsSync() to begin with.

[dfabulich]

@trevnorris I've read all the threads on this. I get it. That's why this PR #4077 adds the "throw error on EPERM" stuff. And that's also why I support merging it. (I agree with those who say that it would require a semver bump. To me, that sounds like a great addition to Node 6.)

#4077 is our best bet. My PR #4217 is an acceptable second-best option.

[bjouhier]

OK, there are plenty of edge cases where fs.existsSync can be problematic (race conditions, access rights) but there are also plenty of scenarios where the edge cases don't matter.

In our application we use fs.existsSync (and fs.exists too BTW) in situations where 1) we are guaranteed by design to have full access on the files (simply because they are inside a directory which is completely controlled by our application) and 2) we are safe by design from race conditions (in some cases because files will not get deleted anyway, in others because concurrent operations each work with their own sandbox of files, ...) For all these scenarios the simple API does the job and it has the advantage of stating explicitly what it does (which is unambiguous because the edge cases are irrelevant).

I think that a large part of the disagreement comes from a difference of perspective between library developers and application developers:

• As a library developer, you don't know the precise conditions in which your library will operate. So you have to be ready for the most hostile case: race conditions, wrong permissions.
• As an application developer, you control how your code will be deployed; you can guarantee that operation xyz will only scan files that the application can always read and write. If not, it just means that something has compromised the deployment of the application (and you're likely to have a security issue). Same thing for race conditions.

You are trying to prevent library implementers from shooting themselves in the foot. Fine. But don't forget the application developers. For them backward compatibility may be more important. It is important that their code, and their dependencies, which might not be all new and shiny, don't break because of a missing API.

[trevnorris]

@dfabulich The "throw in EPERM" is why existsSync() seems less useful. Users should be given a method that is guaranteed to never throw (i.e. if it throws then there's a bug). Otherwise you'll still have to wrap the call in a try/catch. In which case users might as well be left to just wrap statSync() in the try/catch and existsSync() remain deprecated. Hence why a call like accessibleSync() makes more sense.

[crrobinson14]

@trevnorris I don't understand that position. The "other" core dev position was "just try to open the file, and try/catch that." How is try/catch a problem here but a recommended answer there?

I don't understand all these protectionist positions. Bad developers will write bad code - in any language. I've found security holes in Erlang projects, Ruby projects, Scala projects, C# projects... Every one of them was suppose to be "more secure than that awful PHP." Sooner or later it's not the language's job to fix everything a human might do. @bnoordhuis in PR #4217 said basically "you can't have it because it's a TOCTOU pathway and you can't be trusted with this awesome responsibility." Are you going to stop allowing people to run Node as root, too? That's WAY more dangerous, and not discussed at all.

There's a huge pile of stuff in NodeJS that throws. They're well documented and understood, and the developer's fault if they don't catch it. EPERM here is a LITTLE weird to me... but WAY less weird than deprecating existsSync in the first place. If I just want to know if the file exists, I maintain that it's less clear, less readable, and less maintainable to rely on a side effect of another function to determine that when there was a perfectly good function already that handled it. You're spending a huge amount of time here figuring out how to find alternatives for something people want to do, solely because you're deleting an already functional way that they have today.

I can see what's going to happen here. There's going to be a hugely-upvoted Q&A on StackOverflow about this. The most popular answer is going to be a monkey-patch that still has all the same issues as the original.

Maybe that's the best answer for now - it's certainly what I'm going to use. I check for file-existence a LOT without immediately wanting to open and read the file. (Which you still have to try/catch.) It's a valid and useful pattern to me, and I have no intention of saddling some future maintainer with an ugly comment that says "This call to accessibleSync doesn't really need access to the file. We're just seeing if it exists, but since the NodeJS devs took away existsSync we're calling this instead. So just ignore the fact that it says accessible..."

[matthew-dean]

Also, as a side note: the original call to deprecate fs.exists{Sync}, and the PR to remove it had the same response from developers. So none of this should be a surprise to NodeJS devs. It's never been a popular decision. Meanwhile fs.watch is monkey-patched all over the place because it doesn't function reliably, but no one has suggested deprecating or fixing that one. Of course, if the history of these methods are a guide, fs.watch would be removed, and the suggested solution will be to continuously call fs.open to see if the file has changed.

[trevnorris]

The "other" core dev position

We don't always agree. ;-)

I don't understand all these protectionist positions. Bad developers will write bad code

My position isn't about that. It's that the API itself is fundamentally flawed. The point of existsSync() is to prevent needing to wrap the call in a try/catch. That's literally all the call does. So if there is any case where existsSync() can throw then the whole point of it is defeated.

If I just want to know if the file exists

The point I'm trying to make is a file "just existing" is ambiguous. Leading to one reason why EPERM would need to be thrown. Knowning whether the file is "accessible" is better defined, and more easily determined.

(Which you still have to try/catch.)

Then what does it matter? existsSync() is a statSync() wrapped in a try/catch. That's literally it. So replacing the call doesn't change anything else in your code.

I have no intention of saddling some future maintainer with an ugly comment that says...

The documentation states:

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.

So it achieves for all intents and purposes the same thing but is better defined. With the added bonus of being more performant.

[matthew-dean]

@trevnorris@crrobinson14 I think the better solution would be to preserve existsSync, but return undefined when existence can't be determined because of a permission or other access error. That way you could do:

if(!fs.existSync('foo.txt')){ ... // Doesn't exist or isn't accessible - backward-compatibleif(fs.existSync('foo.txt')===false){ ... // No access error thrown, but no file foundif(fs.existSync('foo.txt')===undefined){ ... // Access error thrown when trying to determine existenceif(fs.existSync('foo.txt')){ ... // Hey, look, there's a file there! Cool.

I think this is a slightly better pattern than just a true/false return for accessibleSync.

[crrobinson14]

@trevnorris I get most of what you're saying except this part:

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.
So it achieves for all intents and purposes the same thing but is better defined. With the added bonus of being more performant.

It sounds like you're suggesting

// NOTE: We don't care if the file is accessible, just if it exists. This is the only
// way to do that in NodeJS now. Remember, accessibleSync also tells you it
// exists if you specify fs.F_OK...
if (fs.accessibleSync(filePath, fs.F_OK)) {

or

// NOTE: We don't care if the file is accessible, just if it exists. This is the only
// way to do that in NodeJS now. The default behavior of accessibleSync if no
// options are specified is not to check accessibility at all, just existence...
if (fs.accessibleSync(filePath)) {

are more readable and better practice than:

if (fs.existsSync(filePath)) {

No reminder to future maintainers about your intentions with the last option. This is what I'm stuck on. It just seems so awkward to me. I feel like I need to clarify my intentions to future devs in every spot I do this because they may not realize at a glance that I'm only checking existence.

I do get the try/catch comment. I totally get the fs.exists() (non-sync) deprecation on the API-incompatibility grounds, although I still hold that if you're going to make a breaking change (deprecation/removal), fixing it would have been better than removing it. What I don't get is the position that any of the other options are objectively better, more readable, or more maintainable.

[crrobinson14]

@matthew-dean Actually I'd side with the core devs on that point. My position is that using side effects as primary logic pathways is unclear. Having to === against specific falsy return types to differentiate between exists-inaccessible and not-exists feels awkward to me - actually, it feels like what a lot of people complain about PHP doing.

I can totally see the value of fs.accessibleSync. I would use it - I like the concept. It's very clear at expressing what you're trying to accomplish in code when accessibility is your goal (accessibility being the sum of exists and permissions-allow-X-operation). I just still think deprecating fs.existsSync is a needless and frustrating revision that doesn't add as much value as it takes away.

[matthew-dean]

@crrobinson14 I can respect that. I'm just trying to complain less and find something closer to an acceptable middle ground. If accessibleSync were to never throw an error, I think that would work for me. OR some other non-throwy method that returned a queryable object, like:

if(fs.safeAccessSync('foo.txt').accessible){ ...
// orif(fs.safeAccessSync('foo.txt').exists){ ...
// orif(fs.safeAccessSync('foo.txt').writable){ ...

Something that was a safe and concise utility method that could easily return existence (or accessibility, or both) without throwing errors - to me, THAT would be a perfectly fine replacement for existsSync, because it could, as you point out, be self-documenting.

(Although, of course, "safeAccessSync" is a horrible name.)

[crrobinson14]

@matthew-dean@trevnorris Just a suggestion, what if you added an fs.F_EXISTS alias of fs.F_OK? I'm not in LOVE with losing existsSync but I would certainly be happier writing

if (fs.accessibleSync(filePath, fs.F_EXISTS)) { ... }

than relying on default behavior that does the same, or using F_OK. It's longer than writing existsSync but at least it's very clear what you're doing, and would dovetail nicely with what others are pushing for via accessibleSync...

[bjouhier]

I have a proposal:

1. keep fs.existsSync and fs.exists as they are. Don't deprecate them, don't remove them, don't fix them.
2. design new APIs that will have all the good properties. Take your time and do it right.

People who use the old (bad) APIs will be happy: their code will continue to work.

You will be happy: people who want the old APIs to stay around won't be polluting your debates.

Once the new APIs are out, we'll see. If 2 years down the road a large majority of modules have switched to the new APIs, then it might be time to deprecate the old functions. If, on the other hand, the new API got poor adoption, then the old functions should stay.

Let time (and the Darwinian process) sort this out. There is no reason to rush.

[amb26]

No reminder to future maintainers about your intentions with the last option.

@matthew-dean - yes, I agree completely. The point is that existsSync, regardless of its safety/consistency/efficiency, is a compact and easily recognised means of conveying intention.

[matthew-dean]

@crrobinson14 I thought of something like that too. F_OK is kinda non-specific, since in English it can just mean, "Yeah I heard you... And?" That is, it's not necessarily an "affirmative" or boolean type of word. So yes, I think this would be perfectly fine.

if(fs.accessibleSync(filePath,fs.F_EXISTS)){ ... }

And, hey, you know what? If one were being really clever, one could add a bit of sugar to the above method and write:

if(fs.existsSync(filePath)){ ... }

:P But, look, if it were the first option, I would be fine.

[matthew-dean]

@bjouhier This also seems reasonable. If a clearly better approach DOES manifest, then like I've said, yeah for sure, I would happily say goodbye to existsSync. I just haven't seen it. After all, how do you get more clear and concise than that?

[trevnorris]

It seems the word "access" is leading to incorrect conclusions, like:

(accessibility being the sum of exists and permissions-allow-X-operation)

Which bring the need to explain the usage of API in context, such as:

Remember, accessibleSync also tells you it exists if you specify fs.F_OK...

With the fs.F_OK flag the check is about visibility, or potential to access. Directly from man access(2):

F_OK tests for the existence of the file.

On Linux at least if a file can be viewed then it can be potentially accessed. Thus it exists. But this is not true on Windows. Hence why existsSync() is not cross-platform friendly. It's fundamentally flawed. The original plan was to replace the call to statSync() with accessSync() until this was realized.

My only goal here is to make sure the API is well defined across platforms. existsSync() doesn't do that.

[dfabulich]

@trevnorrisexistsSync that throws on EPERM is well defined on all platforms. You argued earlier that if existsSync can throw then it "defeats the point of using it." But that's not true. If existsSync throws, but only in rare/exceptional circumstances, then developers can choose whether to wrap it in a try/catch block or whether to take their chances.

I'd use it. In most scripts, I'd use it without wrapping it.

[Fishrock123]

I don't really know where to go with this, so I'm gona close it.