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

ensureDir and ensureDirSync are racy #3233

Closed
lucacasonato opened this issue Mar 6, 2023 · 4 comments · Fixed by #3242 or #4041
Closed

ensureDir and ensureDirSync are racy #3233

lucacasonato opened this issue Mar 6, 2023 · 4 comments · Fixed by #3242 or #4041
Labels
bug Something isn't working

Comments

@lucacasonato
Copy link
Member

They do lstat then mkdir rather than doing only mkdir and throwing an error.

If you call ensureDir quickly enough in parallel, one of the two ensureDir's will throw "AlreadyExists".

"exists" checks using something like lstat are wrong 99% of times, and this is no exception :)

@kt3k
Copy link
Member

kt3k commented Dec 28, 2023

We switched the order of mkdir and lstat in #3242, but that caused another DX problem. Now ensureDir always asks for write permission to the given directory even when the directory already exists and it actually doesn't need to write to it.

Now a PR was created to mitigate that situation, but the solution looks very complex. #4008

Should we follow the direction of #4008 or should we revert back to pre-#3242 state, accepting some racy nature of it?

(In my view, ensureDir is usually called for creating special purpose paths such as cache dir, config dir, etc. I can't imagine ensureDir is called in high concurrency. We can also document that it can be racy in highly concurrent situation)

@iuioiua
Copy link
Contributor

iuioiua commented Dec 28, 2023

Accepting the racy nature of the function seems more reasonable and better than a complex solution as long as there's clear notice within the documentation, like exists().

@lucacasonato
Copy link
Member Author

These functions should not be racy. If that means you sometimes need write permissions instead of read permissions, that seems like a reasonable trade-off to me.

@lucacasonato
Copy link
Member Author

@kt3k i opened this bug because I was using it in high concurrency and my code would randomly crash. I think this function being racy is not acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants