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

bzoz · 2018-02-07T11:52:20Z

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

bzoz · 2018-02-07T11:55:45Z

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

richardlau · 2018-02-07T14:05:06Z

lib/fs.js

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.

bnoordhuis · 2018-02-07T17:21:45Z

lib/fs.js

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

BridgeAR · 2018-02-08T20:32:15Z

@bzoz this needs a rebase

evanlucas · 2018-02-09T15:26:48Z

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

bzoz · 2018-02-09T15:29:41Z

@evanlucas : maybe #4679 ?

evanlucas · 2018-02-09T15:51:58Z

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

daynin · 2018-02-10T09:17:54Z

@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

bzoz · 2018-02-12T12:36:54Z

Rebased, PTAL.

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

bzoz · 2018-02-14T14:26:11Z

Failures look unrelated.

BridgeAR

LGTM, just left a suggestion.

BridgeAR · 2018-02-16T21:57:28Z

lib/fs.js

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


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.

bzoz · 2018-02-22T09:57:09Z

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

bzoz · 2018-02-22T11:40:38Z

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>

MylesBorins · 2018-03-06T21:22:57Z

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>

bzoz · 2018-03-28T11:25:15Z

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>

MylesBorins · 2018-08-07T22:26:17Z

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 pending requests for changes, 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 pending requests for changes, 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

Uh oh!

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

richardlau Feb 7, 2018 •

edited

Loading