fs: add accessibleSync() which returns a boolean

[dfabulich]

As opposed to accessSync() which does nothing or throws

[dfabulich]

In #4077 (comment) @domenic wrote that he'd consider a PR like this. In #4077 (comment) I argue that we shouldn't merge this PR; we should merge pull request #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.

[dfabulich]

It seems like there are really three criticisms of exists/existsSync.

One criticism was that exists doesn't use nodeback, which is a non-issue for existsSync.

The second criticism is 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.

The third criticism is that both exists and existsSync are vulnerable to race conditions when misused. You might check whether a file exists and then immediately perform an operation on that file, when you should just try to perform the operation and handle errors. This is not improved by access/accessSync; you might access the file and then try to operate on it. The race condition problem is not improved by this PR either. The race condition argument is a red herring, having nothing to do with whether we call it accessibleSync or existsSync.

All of the arguments against existsSync are inappropriately overcautious. 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.

If we insist on defending the user from these very rare and unimportant edge cases, as a script author, I suppose I just want a boolean, and I don't really care what the function is called. Must we clutter the API to have something easy to use?

Challenge accepted.

[bnoordhuis]

If we insist on defending the user from these very rare and unimportant edge cases

I'd be more sympathetic to that argument if history hasn't shown again and again that people get it wrong. It's 2015 and TOCTOU bugs are still some of the most frequent sources of CVEs.

[dfabulich]

The user will not be protected by renaming the function. access and accessSync are equally vulnerable to TOCTOU. accessSync is just more annoying to use than existsSync or accessibleSync.

[domenic]

Yeah, we should probably just remove it entirely and keep existsSync deprecated.

[dfabulich]

@domenic By "it" you mean removing accessSync? and access? Please don't do that.

Since access follows nodeback and doesn't incorrectly assert non-existence in the case of inaccessibility, the only reason you'd do that is to prevent TOCTOU, right? So you'd delete access just to prevent TOCTOU?

For humor value, here's my most recent use case for existsSync: I wrote a git pre-commit hook as a node script. I needed to handle merge commits differently from non-merge commits. The way for a git pre-commit hook to know if you're currently running a merge is to check to see if .git/MERGE_HEAD exists. If it does, then you're in a merge; if it doesn't exist, you're not.

if (fs.existsSync('.git/MERGE_HEAD')) {

If node deletes access and accessSync, I'll close this PR and open another PR for fs.canStat that returns a boolean if the file can be stated and false otherwise. And if node declines that PR, then we're all just going to do it in userland.

This function will be used. Nothing we say here can stop it.

We can only choose whether to let developers do what they want with a minimum of pain or slow developers down and waste their time.

[crrobinson14]

Wait, we've gone from "existsSync is an anti-pattern" to "it's a TOCTOU source?" Two questions:

1. Do you have any data that backs up the statement that file-exists checks are "some of the most frequent sources of CVEs"? I found barely a dozen in 2015's CVE list and none were in NodeJS apps. In the last 3 years, only 145 vulnerabilities out of 74k even use the term "exists", and nearly always as part of the phrase "this vulnerability exists...".
2. As noted by @dfabulich, accessibleSync and accessSync don't address TOCTOU. Is it the position of the Node maintainers that it's their job to prevent developers from making mistakes? There's all kinds of stuff in fs with caveats about ways in which using them wrong would be bad.

[rvagg]

I'm -1 on this because:

1. This is sugar that just increases our API surface area. There's no reason that we can't have a rimraf or mkdirp style package that does adds this kind of functionality in a cross-version way—that's how we deal with most stdlib questions in modern Node-land.
2. Using sync functions means dealing with thrown errors, that's just the way it works. fs.exists() wasn't just an odd API in core in that it didn't follow the standard Node callback pattern, it also didn't follow the throw-on-error convention in its sync version. We're better for removing those kinds of discrepancies (mind you, I wasn't involved in the decision to deprecate fs.exists() and am not sure I would have been +1 on its deprecation if I was).
3. The lack of API symmetry, sync functions should have non-sync partners, if you can't justify the non-sync version then it calls into question the justification of the sync version (see point 1).

Having said all that, thanks for the contribution @dfabulich. I believe this would be your first core commit if it manages to get in, we're very grateful for the effort regardless of the outcome!

[trevnorris]

For the sake of being explicit I'd make sure that if mode === undefined then you pass fs.F_OK.

[Fishrock123]

Isn't that access's default?

[trevnorris]

I'd lie to think there was a specific reason I suggested this, but can't think of it.

[trevnorris]

@dfabulich access/accessSync won't be removed from the API. They're here to stay.

@rvagg Quick history of how we arrived here: I wanted to find a way to not do a stat call in existsSync. @saghul mentioned the access system call as a faster alternative. @cjihrig added the API uv_fs_access() to libuv to accommodate this. We added the fs.access{Sync} API. An attempt was made to swap the impl in fs.exists{Sync} but we found there were edge case differences so it was never done. fs.exists{Sync} was deprecated. @Fishrock123 created a PR to fix a permissions edge case issue on Windows in exists{Sync} (possibly the same edge case that prevented the original impl change). @domenic proposed adding this API instead and I was in agreement because the API was more explicit and didn't suffer from the gotcha from exists{Sync}.

Overall I don't see an issue having an API like this in core. There's a near zero additional burden on core developers to have it in (taken from the history of exists{Sync}), the code addition is trivial and there's been plenty of community outreach that this API has been important. Since exists{Sync} was deprecated I figured this was an appropriate time to use the more solidly defined API. If you'd like a cooresponding async API then it would be easy enough to add fs.accessible(). If we decide not to go this route then I say we merge @Fishrock123's PR and undeprecate exists{Sync}. IMO if devs have an issue with race conditions using this API then they have an issue understanding asynchronous behavior and have a bigger issue. We already well document the potential pitfalls in the docs about using it. Which I feel should be sufficient.

[Fishrock123]

Probably a better idea than my PR, I don't object to this.

[sam-github]

I don't understand, portable node code is just using a wrapper that calls accessSync if it exists, existsSync if it does not.

I agree with the analysis in #4217 (comment)... except for the conclusion!

• exists didn't use err-back, so needed replacement by a name() and nameSync() pair more consistent with node calling conventions: name was chosen as access, so all is good
• access has better semantics than exists, we are good there
• access has all the race condition possibilities that access() (edit: I mean exists()) or stat(), or create-exclusive or innumerable other more complex coding errors have, and that's OK

My conclusion is "start using accessSync(), it works fine, wrap it a little if you want".

As this PR shows, its a couple of lines to write a wrapper in your own code (or an npm module) if you want to coerce an exception to a boolean return.

[domenic]

@sam-github some people believe in a big core with some sugar for their use cases, some in a small core. In this case the big-core people have won; in other cases we tell them to use a helper library. What can you do.

[crrobinson14]

@domenic Except the big-core people haven't won. The desired function was deprecated and is being removed and their complaints are being shot down across the board. They've lost.

@sam-github "it works fine, wrap it a little if you want" - the complaint was never about whether it works. It was about how silly it is to have to write the following:

// NOTE: Dear future maintainer, since this isn't obvious I'm letting you know
// that we're not actually checking accessibility here. This is just the only way left to
// check to see if the file exists, by using a side-effect of accessSync that says if we
// supply F_OK, or no parameter at all, we get what existsSync used to do.
if (!fs.accessSync(...)) {
}

existsSync also "worked fine" and was desired and used by many developers who have repeatedly requested/begged for it to be kept. It's not just "sugar" - almost every major language provides this mechanism as a first-class operation because it's concise, expressive, and a very common operation to perform. In #1592, @Fishrock123 asked for examples of what other languages do, for which I provided this summary: #1592 (comment).

This isn't just about "waa, waa, I want a big core". It's about who Node serves. What the dev community is learning here is that the core contributors are more focused on serving a much smaller, API-server-focused use-case where these kinds of operations are rarely performed and thus considered unimportant. If that's true, that's fine - but you shouldn't pretend otherwise.

This doesn't come down to "you can find this information via another call, just use it." It comes down to "if you want to code expressively for these kinds of operations, you should chose a language like Python or Ruby that allows you to do it directly." That's the real message you send when you tell developers "just wrap it." It's not enough to offer "some sort of solution" to a problem. You have to offer a GOOD solution for a platform to be usable and desirable by developers.

[sam-github]

@domenic Yeah, I am aware of the tension. But big-core hasn't won in the fs module [*], and making "access" a ternary API, with a third option that converts an exception to a boolean, isn't a pattern that exists elsewhere in fs. Or indeed, elsewhere in node, so it creates another fs API that is uniquely hard to document and justify, decreasing the sensibility and coherence of the API. IMHO, of course.

• unless you count the existence of streams support - I'm personally of the opinion streams shouldn't be in core :-). Clearly I'm an advocate of a smaller core.

[domenic]

Yeah, but as you can see, if you advocate for a smaller core you will be greeted with a wall of text about how you are not serving developers, so at some point it's easier to just accede IMO.

[sam-github]

@domenic I'm not giving up quite yet - still pushing, gently!

[dfabulich]

I actually also favor small core. That's why I just want existsSync to be undeprecated! My "big core" PR only exists because folks said they didn't want to accept the small core option.

But the problem is that accessSync is a painful API. So we can undeprecate existsSync and have a small good core, or do nothing and have a small bad core, or land this PR have a big core.

If I have to choose between a small bad core and a big core, then I'll hold my nose and have a big core.

[sam-github]

Small and bad has endless price to be paid in terms of "WTF moments" for new users. So, never breaking compatibility is good for existing users, bad for everyone who comes after. More people are coming than are here, lets make it better.

@dfabulich a 3 line wrapper is that painful to you? And a check-exists npm module is out of the question? You are that opposed to composable small modules?

[dfabulich]

I agree that the goal is to minimize WTF moments for future users. Today, it looks like this:

A1) ⌘F the fs docs for exists/existsSync. WTF, it's deprecated?
A2) WTF is access? That doesn't sound like what I what I want.
A3) WTF accessSync doesn't even return a boolean?!
A4) (Maybe) WTF I was TOCTOU'd!

If we undeprecate existsSync:
B1) ⌘F the fs docs for existsSync. The developer thinks that's what she wants, and uses it.
B2) (Maybe) WTF it returned false on Windows because I didn't have permissions!
B3) (Maybe) WTF I was TOCTOU'd!

Now, let's suppose check-exists were a thing. Would we then actively recommend it in the documentation? Like this?

fs.existsSync is deprecated. Use npm check-exists instead, which is a 3-line wrapper around accessSync.

But check-exists is going to have all of the TOCTOU race conditions of access and exists and its semantic problems (returning false negatives when the file exists but is inaccessible).

So it looks like this:
C1) ⌘F the fs docs for existsSync. WTF it's deprecated?
C2) WTF the replacement isn't in core?
C3) (Maybe) WTF check-exists returned false on Windows because I didn't have permissions!
C4) (Maybe) WTF I was TOCTOU'd!

That's not really an improvement over existsSync. It's like locking the door and posting a sign, "please crawl in through the basement."

Relatedly, we could perhaps take some of the edge off of A3 by providing the wrapper as an example in the documentation for accessSync.

If you'd like to use accessSync as an alternative to existsSync, you'll need to wrap the call in a try/catch block, like this:

try {
  fs.accessSync(path);
  return true;
} catch (e) {
  return false;
}

Documentation like this is an embarrassment. It would put a piece of code in the doc that we reasonably expected every user of the API to copy and paste. Forever. How painful is that?

I think you'll find that if (by a quirk of history) the core already had accessibleSync and we were arguing about whether to add accessSync, you'd find that almost nobody would want to use accessSync. The only thing accessSync does is sometimes throw an exception; the only reason you'd want to use it over accessibleSync is to interrogate the thrown error (e.g. check its error code).

So, how often would people do that? I paged through a few pages of https://github.com/search?q=accessSync&type=Code&utf8=%E2%9C%93 and couldn't find anybody who interrogated the thrown exception; they all just re-implemented the try/catch wrapper.

a 3 line wrapper is that painful to you?

Add up all the hours we've spent arguing about this, and every second that will ever be spent in the future maintaining this trivial function; developers have already wasted that much time and more learning about the problem and re-implementing the wrapper themselves.

That painful.

[crrobinson14]

I agree with @dfabulich.

@sam-github: Yes, npm install to add three lines of code to a project IS a burden. npm install is for "Node Modules". A "module" should be a significant piece of functionality. algoliasearch, sequelizejs, and aws-sdk are modules. Three lines of code is not a module. Modules should concentrate a lot of related functionality in a single package with a shared namespace for accessing that functionality. Installing modules adds overhead to package management and maintenance when doing deployments/upgrades of applications. It's silly to move a single function out to a module, particularly one whose only role is "make NodeJS not look backward compare to other languages."

fs is a module. It is a module that provides filesystem functionality and has very strong analogs to similarly named packages in Python, Ruby, etc. Except for one thing - a very popular and commonly-used function was dropped from it to make it "smaller".

Why not go all the way and drop fs entirely - make THAT the npm add-on? Where do you draw the line? Why does fs include watch, whose own documentation says The fs.watch API is not 100% consistent across platforms, and is unavailable in some situations. and yet this is an acceptable core API function, but existsSync is not? Why does fs include lchmod, which is Only available on Mac OS X? watch is so similar to inotify based APIs and libraries that you could easily argue it's the better candidate for moving out, particularly since its behavior is so unpredictable cross-platform.

I have yet to see any developer step up and say "My life/code will be made better by this deprecation. That old function was stupid, anyway. I prefer calling "access" when I mean "exists" - that's totally clearer. Thanks, folks!" If that's not a measure of whether value is being added or subtracted here, I don't know what is.

[dfabulich]

I have another proposal that might be ever so slightly more likely to make progress.

1. today's implementation of existsSync wraps a try/catch block around a synchronous call to stat. existsSync, even if deprecated, should probably use a synchronous access anyway, for performance reasons.

2. When accessSync fails, it throws an exception. I bet constructing that exception comes at a measurable performance cost (constructing a stack); if we know that users are just going to throw away the exception and return false, then it's a total waste.

Instead of the trivial implementation we have here, we (I?) could make a larger PR that allows the binding to return a boolean. That would make existsSync and/or accessibleSync faster than the 3-line wrapper, in addition to having better ergonomics, in a way that can only be done well in core.

3. But if we did that, then the only way to expose that benefit to users would be to either undeprecate existsSync or add accessibleSync to core. I remain optimistic that at that point we'd undeprecate existsSync, but if that's politically impossible, well, we'd just have to add accessibleSync at that point, to get the performance benefit.

WDYT?

[trevnorris]

@dfabulich

existsSync, even if deprecated, should probably use a synchronous access anyway, for performance reasons.

Please read my brief history above (#4217 (comment)) of why this isn't possible.

I think you'll find that if (by a quirk of history) the core already had accessibleSync and we were arguing about whether to add accessSync, you'd find that almost nobody would want to use accessSync. The only thing accessSync does is sometimes throw an exception...

You say this as if no one would ever want to know if a file was readable or writable. Might want to reread the options that an be passed.

I prefer calling "access" when I mean "exists" - that's totally clearer.

Access is actually more correct when taking a filesystem into consideration. Windows has shown us that existence and accessibility are not the same. This PR just restricts option usage to F_OK, which basically states "can I potentially access this file or not". This is a more technically correct cross platform way to perform this type of operation.

[sam-github]

@crrobinson14 In other languages, facilities like https://www.npmjs.com/package/mkdirp are also in their "fs" libs... but not in node, because they can be implemented in pure js on top of the core API.

Not everyone agrees on the "needs a compiler" test and a smaller more maintainable core, you don't! But the distinction is not arbitrary.

To answer your specific questions:

Why not go all the way and drop fs entirely - make THAT the npm add-on?

Because it requires a thread pool and interaction with the system/event loop.

Where do you draw the line?

Requires a compiler vs can be implemented in pure js.

Why does fs include watch, whose own documentation says The fs.watch API is not 100% consistent across platforms, and is unavailable in some situations. and yet this is an acceptable core API function,

Because its widely requested, and can be used cross-platform if you are very careful. And requires a compiler and deep integration with the event loop. Many Node.js APIs, btw, are not 100% consistent across platforms. 100% consistency would mean "lowest commmon denominator", and Node.js is better than lowest, though it does mean coding carefully if you need portable fs or child_process code.

but existsSync is not?

Is a trivial js wrapper around underling fs APIs.

Why does fs include lchmod, which is Only available on Mac OS X?

Requires a compiler.

Efforts are underway elsewhere (with websockets, for example), to integrate minimal amount of code into Node.js to allow user-land modules to be written in pure-js.

[dfabulich]

This PR just restricts option usage to F_OK

That's not right; this PR #4217 allows you to run any of the accessibility checks, not just F_OK.

You say this as if no one would ever want to know if a file was readable or writable.

No, I mean in a world where accessibleSync was already a part of core, people would use accessibleSync(fs.W_OK) to check writeability and go on their merry way.

The only people who would say, "accessibleSync(fs.W_OK) is not enough for me; I need accessSync" would be people who'd need to inspect the error code. But, I claim, those people basically don't exist. They certainly don't appear to exist today based on a cursory code search.

I take that to be evidence that accessibleSync is the API we should have had all along, especially if it can be made to be faster than accessSync-with-a-wrapper due to performance improvements that require a compiler.

As for your brief history, you didn't state exactly why existsSync wasn't ported to use access, except to gesture toward "edge cases"; you speculated that the EPERM PR was "possibly" the same reason.

I think changing existsSync's edge-case behavior would be fine for a major update, e.g. Node 6, if it would also improve performance and keep the core small.

[trevnorris]

That's not right; this PR #4217 allows you to run any of the accessibility checks, not just F_OK.

Oop. Yup. Thanks for the correction. Responding via phone diminished my context.

I take that to be evidence that accessibleSync is the API we should have had all along,

That's a well put way for what I was getting at.

As for your brief history, you didn't state exactly why existsSync wasn't ported to use access, except to gesture toward "edge cases";

Sorry. Too lazy to dig through my IRC logs and find the specific discussion. It boiled down to lack of cross platform consistency. Issues like what @Fishrock123's PR addresses.

[Fishrock123]

re: "small core" vs "big core"

This is in essence fixing something that already exists.

[dfabulich]

I claim, at a minimum, that there is no "cross-platform" consistency issue with exists/ existsSync except the Windows EPERM issue. If any developer knows otherwise, please bring forth another example.

There is a separate concern which has nothing to do with cross-platform support, about checking for the existence of a file on an inaccessible volume (e.g. removable media). exists/existsSync returns false in that case, on all platforms, even though that's not really knowable. All we know is that the file is inaccessible. In an earlier comment I said that exists semantically conveys "too much certainty." It's an epistemological argument.

Even the Windows EPERM issue is just a special case of the "we can't be sure whether it exists or not" case. A directory you don't have the right to read is about the same as a directory that's no longer connected to the current machine.

I've argued today that we have three options:

1. small bad core: do nothing
2. small good core: undeprecate existsSync
3. big core: add accessibleSync

I claim that anyone who agrees that #1 is the wrong thing should not be motivated to do #3 for purely epistemological reasons. Even granting that accessibleSync is more epistemologically correct than existsSync, existsSync is at least pretty close to right, and already exists in core. If exists/existsSync were already implemented based on access, and someone tried to present a PR to deprecate exists and existsSync and replace them with access and accessSync based only on the epistemological argument above, I hope they'd be politely but soundly rejected.

Demanding the perfect name for every function paves the road to big core. Plenty of methods in core have questionable names; it would be chaos to deprecate them all.

But I claim that the epistemological argument is, in turn, the only justification for #3 over #2. So if you agree that the epistemological argument alone shouldn't convince us to embiggen core, then you should agree that we shouldn't embiggen core with accessibleSync.

Either we should do nothing, or we should make existsSync do the best job it can. It's been deprecated since Node 4. Converting it to a fast boolean access call in Node 6 (or Node 7 or 8, if it comes to that) is the best way forward. It'll be a tiny bullet in the "breaking changes" list that shows up on every major Node version, and you'll never even hear a complaint about it, because the 0.001% of users who are testing for the existence of files on removable media (or directories they have no right to access) can't really complain that existsSync returns false in that case.

The behavior of this deprecated function isn't set in stone, immutable for all time. We can make this better in a major version bump. Let's do it.

[trevnorris]

@domenic In light of big/small core, remember you were the one that initially suggested accessibleSync(). :)

@dfabulich I'd be alright with swapping the impl of exists() to use access as a major change. That was mostly my original intent way back.

[jasnell]

@nodejs/ctc ... where are we at on this one? I know we discussed it last week on the call but we weren't able to settle on a decision as far as I recall.

[rvagg]

See notes: https://github.com/nodejs/node/blob/master/doc/ctc-meetings/2016-01-06.md#gripe-deprecating-fsexistsexistssync-1592

And comment: #1592 (comment)

I believe it can be bundled as a single issue for now? Perhaps pulling it apart at some point might make it easier to find resolution(s)?

[dfabulich]

I implemented my proposal from Jan 4 to improve the performance of accessibleSync beyond what's currently possible in JS, by avoiding throwing an ignored exception. I also filed PR #7455 on this.

I hope that we merge this PR #7455, but if the team refuses to merge that PR, then I hope one of #7455 or #4217 can be merged.

[Fishrock123]

closing in favor of #8364 -- please let me know if this was in error.