Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Somehow support checking if a path exists while using fs.promises #39960

Closed
bnb opened this issue Aug 31, 2021 · 32 comments · May be fixed by #39968
Closed

Somehow support checking if a path exists while using fs.promises #39960

bnb opened this issue Aug 31, 2021 · 32 comments · May be fixed by #39968
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. stale

Comments

@bnb
Copy link
Contributor

bnb commented Aug 31, 2021

Is your feature request related to a problem? Please describe.

Presently, fs.exists is deprecated and fs.existsSync is only available if you're not using promises. The exists check is absolutely useful, and it would be really useful to have it available in the fs.promises API.

Describe the solution you'd like

fs.promises,exists or fs.promises.existsSync should work.

Describe alternatives you've considered

  • Status Quo: this leaves a weird and very unfortunate gap in our API.
@devsnek
Copy link
Member

devsnek commented Aug 31, 2021

async function exists(f) {
  try {
    await fs.promises.stat(f);
    return true;
  } catch {
    return false;
  }
}

😄

@aduh95
Copy link
Contributor

aduh95 commented Aug 31, 2021

Alternatively, you can use fs.promises.access which is not exactly the same thing, but may fit your use case.

@VoltrexKeyva VoltrexKeyva added the fs Issues and PRs related to the fs subsystem / file system. label Sep 1, 2021
@Ayase-252 Ayase-252 added the feature request Issues that request new features to be added to Node.js. label Sep 1, 2021
@targos
Copy link
Member

targos commented Sep 1, 2021

I think it makes sense to implement fsPromises.exists. fs.exists is only deprecated because of its non-conventional callback signature.

Ayase-252 added a commit to Ayase-252/node that referenced this issue Sep 1, 2021
Ayase-252 added a commit to Ayase-252/node that referenced this issue Sep 1, 2021
Ayase-252 added a commit to Ayase-252/node that referenced this issue Sep 1, 2021
@tniessen
Copy link
Member

tniessen commented Sep 1, 2021

fs.exists is only deprecated because of its non-conventional callback signature.

Historically, that might be true, but exists is problematic for another reason. It often incorrectly returns false. The name is misleading at best.

> fs.existsSync('/root/snap') // existing directory but no permission
false

If we really want to keep using a misleading name, the documentation should be very clear that exists === false does not mean that the file does not exist.

@targos
Copy link
Member

targos commented Sep 1, 2021

Instead of keeping the misleading name, I'd prefer we implement the promise version correctly: return a boolean if we know for sure whether the file exists or not, throw in other cases.

@tniessen
Copy link
Member

tniessen commented Sep 2, 2021

I'd prefer we implement the promise version correctly: return a boolean if we know for sure whether the file exists or not, throw in other cases.

I tend to agree, but that's inconsistent with the existing APIs. And I have a feeling that most use cases won't handle that third case (throw) correctly. Personally, I believe that the semantics of access are not much more complicated than correctly using exists.

@targos
Copy link
Member

targos commented Sep 2, 2021

Personally, I believe that the semantics of access are not much more complicated than correctly using exists.

I'm not sure I understand. Is your opinion that people should use fsPromises.access and that we shouldn't implement fsPromises.exists or that we should implement like the sync version?
The problem with access is that to use it inline (if (exists(somefile)) { ... }), you have to create a helper function to wrap the try/catch.

@tniessen
Copy link
Member

tniessen commented Sep 2, 2021

I don't have a clear preference. The sync version makes a very weak statement: whether some file system entry is visible to the process right now. It doesn't make any statement about the type of the file system entry nor about permissions nor about the entry's existence during the processes' next CPU cycle.

I have a suspicion that the mere existence of exists promotes bad access patterns, such as race conditions (e.g., exists before open or readdir), or checking for existence when the process should really be using access to check for read permissions (e.g., directories), or checking if something exists when the process should really be using stat to check if it exists and is a regular file atomically.

However, there are surely some valid use cases that truly only care about the existence right now and about nothing else.

I don't have a strong opinion. Either throw, which is inconsistent with the other exists implementations and might not be what users expect, or swallow errors, which is misleading/incorrect and requires a big warning in the docs.

access and stat can be used instead of either option and only require one or two lines of code.

@jasnell
Copy link
Member

jasnell commented Sep 4, 2021

The key challenge with exists in the promise API is that it, in absolutely no way, can tell you about the existence of the file right now. The best it can ever do is tell you that the file did exist at one point. If someone really does need to know only about the existence of a file right now the sync API remain the best option.

@tniessen tniessen changed the title Somehow support checking if a path exists while using fs.proimsies Somehow support checking if a path exists while using fs.promises Sep 7, 2021
@tniessen
Copy link
Member

tniessen commented Sep 7, 2021

@ctjlewis Let me remind you of our Code of Conduct that includes "Being respectful of differing opinions" as a community standard. We neither support rude comments nor expressions that are generally considered rude or offensive.

I did acknowledge that there are some use cases that would benefit from exists. I did acknowledge that there are at least two ways of implementing the requested feature. I believe that the concerns that I voiced are valid and shared by others (#39960 (comment), #39968 (comment), #39968 (comment)). I never said that exists should not be implemented, and I voiced my support for the behavior suggested by @targos in #39960 (comment).

Convenience is a valid concern when designing APIs. However, personally, I believe that correctness, safety, security, and consistency are similarly if not more important, and should be considered :)

(Edit: original comment deleted by author.)

@bnb
Copy link
Contributor Author

bnb commented Oct 20, 2021

honestly looking at this issue, it sounds like we should honestly consider removing fs.exists in a future major if the API isn't consistent and will not be dragged into the Promise era.

@bnb
Copy link
Contributor Author

bnb commented Oct 20, 2021

Expanding on the problem a bit, providing a nicer handler for people to use that does what fs.exists says it should do on the box (telling people "use fs.access" feels like an API failure to me) seems like a good approach?

@aduh95
Copy link
Contributor

aduh95 commented Oct 20, 2021

telling people "use fs.access" feels like an API failure to me

Maybe a better response would be "use fs.open", and our effort should focus on making sure that FileHandle API covers most use cases (It seems to me that most of the time folks should use fs.open when they are doing fs.exists).

@targos
Copy link
Member

targos commented Oct 22, 2021

It seems to me that most of the time folks should use fs.open when they are doing fs.exists

I don't know. My use cases for fs.exists usually don't involve opening/interacting with the file.

For example:

if (!exists('node_modules')) {
  run('npm install');
}

run(exists('package-lock.json') ? 'npm ci' : 'npm install');

@jasnell
Copy link
Member

jasnell commented Oct 22, 2021

Perhaps a better approach would be something more like...

await ifExists('node_modules', async () => run('npm install'))

Where the given callback is invoked immediately if the path exists. This type of pattern would avoid the race condition that exists in the current API.

@Ginden
Copy link

Ginden commented Dec 21, 2021

If someone really does need to know only about the existence of a file right now the sync API remain the best option.

This type of pattern would avoid the race condition that exists in the current API.

It's system-level race condition. Another process or thread can delete file or change its permissions.

Though, API with signature like fs.existsAndAccessible(filePath, FileAccess.READ | FileAccess.WRITE) would probably be nice.

@bnb
Copy link
Contributor Author

bnb commented Dec 27, 2021

@jasnell that would solve it, although it definitely feels like an API departure in a way that I'd hope we'd be able to avoid.

@tniessen
Copy link
Member

This type of pattern would avoid the race condition that exists in the current API.

Only for the current thread, though. A worker thread or a different process could delete or create the file in the meantime.

Personally, I don't think the race condition is the main problem. Applications that use access or exists must already be aware of race conditions and the main purpose of exists might be during application startup and/or in automation scripting where race conditions are less likely to be problematic.

My main concern is #39960 (comment): the sync version never throws and misleadingly tells users that a file does not exist when it really just cannot be accessed. An async implementation as in #39968 could either keep that misleading behavior or throw when the existence of the file cannot be determined, but that would then require proper error handling etc., which might not be much simpler than when using access.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Jun 27, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@theoludwig
Copy link
Contributor

This issue should be still considered.
Please reopen the issue.

@tniessen tniessen reopened this Sep 11, 2022
@bnb
Copy link
Contributor Author

bnb commented Sep 11, 2022

My main concern is #39960 (comment): the sync version never throws and misleadingly tells users that a file does not exist when it really just cannot be accessed.

Sounds like a good semver major change :)

@github-actions github-actions bot removed the stale label Sep 12, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 12, 2023
@bnoordhuis
Copy link
Member

I'll put this out to pasture, the discussion here is a little too unfocused. My understanding of reading it:

  1. promisified fs.exists() - no

  2. a better API than fs.exists() - maybe. File a new issue to hash out the details if you want to pursue that.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Mar 12, 2023
@broofa
Copy link

broofa commented Feb 21, 2024

If I may, I'd like to ask that we reopen this. As I understand it, there are two concerns with the exists() API:

  1. The original API (fs.existsSync) incorrectly reports false in some instances.
  2. Being async, there is the possibility of a race condition where a file may be deleted before the promise resolves.

The first is "just a bug". (Air-quotes on that so as not to appear too dismissive. I know "small" API issues like this are hard to deal with in a project of this size.) That can/should be dealt with on it's own merits. For example, I'd suggest leaving the fs.existsSync behavior as-is for now to avoid a breaking change, and implement fsPromises.exists() with whatever behavior is deemed "correct". That the two methods will behave slightly differently may be odd (for the handful of users that notice), but the difference can be noted in the docs. If and when those users complain, fs.existsSync() can be updated to the new behavior as a 𝓫𝓻𝓮𝓪𝓴𝓲𝓷𝓰 𝓬𝓱𝓪𝓷𝓰𝓮.

Where the race-condition issue is concerned, fsPromises is an async API; race conditions are part and parcel to that. For example ...

import fs from 'node:fs/promises';

async function work() {
  return fs.writeFile('test.txt', 'hello world').then(() => fs.unlink('test.txt')).then(work);
}

work(), work();

... will very quickly throw "ENOENT: no such file or directory".)

The rhetorical question being: "Why be okay with race conditions resulting from async writeFile(), but not ones resulting from async exists()?"

The problem with not providing an async exists() is there are numerous cases where neither of the above concerns are all that important. For example, something like fs.exists('/.dockerfile') is the standard check for to see if you're running in a Docker container. But implementing that in node requires a surprising amount of mental gymnastics:

"Should I just use existsSync()? That means I'm importing both fs and fs/promises which is awkward. And why isn't async exists() a thing? There must be a reason for that. ... <googles> ... Okay, I think I get it. But I still want an async exists() so how do I get that? Should I use an inline try-catch around fs.stats()? Or would fs.access() be better? (Does it matter?!) That's a lot of lines of code for something as fundamental as a file existence check, though... so I should probably refactor that into a helper function? But where do I put that? In a util dir? Hmm... what about an npm workspace? Or maybe there's an npm module? That looks good, but does it support TS and ESM? Do I trust the maintainer? Is this worth taking on 3rd party dependency ...?"

My long-winded point here is that lack of fsPromises.exists() is a pretty glaring hole in the fsPromises API. Every time I bump into this, it creates a little twinge of DX pain that it'd be really nice to avoid.

@ohhmm
Copy link
Contributor

ohhmm commented Mar 1, 2024

I created new pull-request with API change:
#50830
This looks like expected and usable API.

@Ginden
Copy link

Ginden commented Mar 4, 2024

@ohhmm It isn't accidentally omitted feature from fs/promises, but deliberate decision.

If you want to get it implemented, I suggest reading these comments:

#39960 (comment)
#39960 (comment)
#39960 (comment)
#39960 (comment)

@ohhmm
Copy link
Contributor

ohhmm commented Mar 11, 2024

@Ginden, there is no way to remove this kind of race condition, except transaction file systems.
Use cases become more complex and expected behavior is missed.
Please, do reconsider.

@ohhmm
Copy link
Contributor

ohhmm commented Mar 11, 2024

If someone want to have secure solution without race conditions then transactions may be emulated by creating file watcher, to monitor if file deleted. This way exists should accept callback and return file watcher.

@stanleyxu2005
Copy link

stanleyxu2005 commented Apr 12, 2024

Personally, I believe that the semantics of access are not much more complicated than correctly using exists.

I'm not sure I understand. Is your opinion that people should use fsPromises.access and that we shouldn't implement fsPromises.exists or that we should implement like the sync version? The problem with access is that to use it inline (if (exists(somefile)) { ... }), you have to create a helper function to wrap the try/catch.

If access() would return true instead of undefined, it will help.
Currently we have to seek alternative solution e.g.

  • using fs-nextra's await fsn.pathExists(filePath).
  • or using a strange oneliner (await fs.promises.access(filePath)) === undefined.

@aduh95
Copy link
Contributor

aduh95 commented Apr 12, 2024

Don’t forget the classic await fs.promises.access(filePath).then(()=>true,()=>false) as suggested in #39960 (comment)

@stanleyxu2005
Copy link

Don’t forget the classic await fs.promises.access(filePath).then(()=>true,()=>false) as suggested in #39960 (comment)

Thanks. Indeed this works. It means you need to create a wrapper function to return boolean value.

It's fine for a simple script, but if it is a real application, such function will be placed in util. I don't see much necessary to maintain the complexity. New node versions has already droped legacy OS support. By adding returning a true instead of undefined, will it bring any historical compatibility issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.