fs: use fs.access in fs.exists by bzoz · Pull Request #18618 · nodejs/node · GitHub

Uses fs.access() to implement fs.exists() functionality. Fixes a issue, when a file exists but user does not have privileges to do stat on the file.

Fixes: #17921

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

fs

CI: https://ci.nodejs.org/job/node-test-pull-request/13007/

Is the behavior of fs.exists altered if callback is not a function? It looks like fs.access throws a TypeError in that case.

cb is a function (it's defined on the line below) and it checks that callback is truthy.

@bzoz this needs a rebase

I feel like we tried to do this a few years ago (sometime around the time we added fs.access{Sync} and had to revert for some reason. I'll look back and see if I can find it

@evanlucas : maybe #4679 ?

Oh well, I can't find it. Nevermind :]

@bzoz hi! Resolve the conflict pls

Uses fs.access to implement fs.exists functionality. Fixes a issue, when a file exists but user does not have privileges to do stat on the file. Fixes: nodejs#17921 # Conflicts: # lib/fs.js

Rebased, PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/13122/

Failures look unrelated.

LGTM, just left a suggestion.

I think it would be good to use a cached fs.accessSync version. That way everything would still work, even if that function gets monkey patched.

That should be picked up in a separate PR, I think.

Since we do not use any reference so far and now introduce one, it might break. So I would rather do that in this PR, but it's not blocking for me.

Yeah, I get the argument but monkeypatching fs is something that already a normal pattern. If we're going to fix it for one method, I'd rather fix it for all of them so there's less inconsistency. I'm happy either way tho. I was going to get this landed but will hold off for now.

So, is it ok for me to land this?

It is OK to land but I personally would say it is better to use a cached version as we otherwise might introduce a potential issue that was not there before.

New CI: https://ci.nodejs.org/job/node-test-pull-request/13325/

Landed in d3955d1

Uses fs.access to implement fs.exists functionality. Fixes a issue, when a file exists but user does not have privileges to do stat on the file. Fixes: #17921 PR-URL: #18618 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Uses fs.access to implement fs.exists functionality. Fixes a issue, when a file exists but user does not have privileges to do stat on the file. Fixes: nodejs#17921 PR-URL: nodejs#18618 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport in #19654

Uses fs.access to implement fs.exists functionality. Fixes a issue, when a file exists but user does not have privileges to do stat on the file. Fixes: #17921 PR-URL: #18618 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Uses fs.access to implement fs.exists functionality. Fixes a issue, when a file exists but user does not have privileges to do stat on the file. Fixes: #17921 Backport-PR-URL: #19654 PR-URL: #18618 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Uses fs.access to implement fs.exists functionality. Fixes a issue, when a file exists but user does not have privileges to do stat on the file. Fixes: nodejs#17921 PR-URL: nodejs#18618 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

The 9.x backport does not land cleanly unfortunately

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 7, 2018

cjihrig approved these changes Feb 7, 2018

View reviewed changes

richardlau reviewed Feb 7, 2018

View reviewed changes

bnoordhuis approved these changes Feb 7, 2018

View reviewed changes

richardlau approved these changes Feb 7, 2018

View reviewed changes

starkwang approved these changes Feb 8, 2018

View reviewed changes

joyeecheung approved these changes Feb 8, 2018

View reviewed changes

BridgeAR approved these changes Feb 8, 2018

View reviewed changes

evanlucas approved these changes Feb 9, 2018

View reviewed changes

jasnell approved these changes Feb 9, 2018

View reviewed changes

daynin approved these changes Feb 10, 2018

View reviewed changes

fs: use fs.access in fs.exists

cb03f5c

Uses fs.access to implement fs.exists functionality. Fixes a issue, when a file exists but user does not have privileges to do stat on the file. Fixes: nodejs#17921 # Conflicts: # lib/fs.js

bzoz force-pushed the bartek-exists-as-access branch from 6c9658e to cb03f5c Compare February 12, 2018 12:35

BridgeAR approved these changes Feb 16, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 16, 2018

bzoz closed this Feb 22, 2018

MylesBorins added the backport-requested-v9.x label Mar 6, 2018

bzoz mentioned this pull request Mar 28, 2018

[v9.x backport] fs: use fs.access in fs.exists #19654

Closed

2 tasks

targos added backported-to-v9.x and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. backport-requested-v9.x labels Apr 4, 2018

targos mentioned this pull request Apr 4, 2018

v9.11.0 proposal #19799

Merged

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

This was referenced Nov 19, 2019

fs.existSync returns true after fs. symlinkSync node 10 onwards in windows only #30538

Closed

fs: fix existsSync for invalid symlink at win32 #30556

Closed

coderabbitai Bot mentioned this pull request Jul 26, 2026

fix(native-chat): resolve WSL Codex transcripts for Chat UI toggle stablyai/orca#10639

Open

2 tasks

-                  if (ctx.errno !== undefined) {
-                    return false;
-                  }
+                  fs.accessSync(path, fs.FS_OK);

fs: use fs.access in fs.exists by bzoz · Pull Request #18618 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: use fs.access in fs.exists - #18618

fs: use fs.access in fs.exists#18618bzoz wants to merge 1 commit intonodejs:masternodejs/node:masterfrom JaneaSystems:bartek-exists-as-accessJaneaSystems/node:bartek-exists-as-accessCopy head branch name to clipboard

Conversation

bzoz commented Feb 7, 2018

Checklist

Affected core subsystem(s)

Uh oh!

bzoz commented Feb 7, 2018

Uh oh!

richardlau Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 8, 2018

Uh oh!

evanlucas commented Feb 9, 2018

Uh oh!

bzoz commented Feb 9, 2018

Uh oh!

evanlucas commented Feb 9, 2018

Uh oh!

daynin commented Feb 10, 2018

Uh oh!

bzoz commented Feb 12, 2018

Uh oh!

bzoz commented Feb 14, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Feb 22, 2018

Uh oh!

bzoz commented Feb 22, 2018

Uh oh!

MylesBorins commented Mar 6, 2018

Uh oh!

bzoz commented Mar 28, 2018

Uh oh!

MylesBorins commented Aug 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

fs: use fs.access in fs.exists#18618
bzoz wants to merge 1 commit into
nodejs:masterfrom
JaneaSystems:bartek-exists-as-access

richardlau Feb 7, 2018 •
edited
Loading