Closed henricavalcante closed 5 years ago
If may make a suggestion the node documentation (see https://nodejs.org/api/fs.html#fs_fs_exists_path_callback) does not recommend this pattern to check if a file exists before writing it as it may introduce a race condition.
They instead recommend using a try
`catch` statement.
May it introduce a race condition even with fs.existsSync()
? This is not an issue only on fs.exists()
?
I believe it is possible for the directory to be created between calling fs.existsSync()
and fs.mkdirSync()
. The OS is allowed to halt execution of a process whilst it does other things (and I think "other things" could include creating a directory). The chance of anyone getting caught by this in practice is incredibly small but concurrency bugs are a very nasty class of problems and to avoid them like the plague leads to a better quality of life for everyone involved.
At least this is what I think. From the docs of fs.existsSync()
:
For detailed information, see the documentation of the asynchronous version of this API: fs.exists().
Therefore, I believe the warnings given for fs.exits()
also apply for fs.existsSync()
.
fs.exists() is deprecated, but fs.existsSync() is not. The callback parameter to fs.exists() accepts parameters that are inconsistent with other Node.js callbacks. fs.existsSync() does not use a callback.
The fact that fs.exits()
is deprecated but fs.existsSync()
is not does not mean that fs.existsSync()
avoids the race conditions but simply that it doesn't have unusual callback parameters.
Yes, I got your point and did some tests and using fs.exits()
to check if a file exists and doing something with that file/folder it may result in a race condition because the file is a shared resource and it can be modified by another process/thread. In this specific case, it will be almost impossible to happen but I will change the code to use try/catch instead of checking if it exists before.
Any update on if this will get merged and released?
Anything else I need to do to get this merged?
Is there a way we can add a test for this to make sure it doesn't regress in the future?
Thanks! This was merged via https://github.com/avh4/binwrap/pull/32
Wrapped the mkdirSync with existsSync to prevent
file already exists…
exceptionit may fix:
https://github.com/avh4/binwrap/issues/29 https://github.com/avh4/elm-format/issues/602