-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use create_new
instead of atomic-file-write
#2914
Conversation
This provides the same functionality but without temporary files, platform-specific code, fragility of `O_TMPFILE` support, and an extra dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this implementation is more fragile than the one it replaces, as it does not consider race conditions, or even what happens on the first run before the file exists.
If two crates in the workspace have the same query, they'll fight over each other trying to delete and then re-create the file if Cargo decides to compile them concurrently.
Good points, I forgot to explicitly handle these conditions as non-errors. I think the latest commit should handle these properly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced that avoiding the dependency is worth it, but I guess there's nothing obviously wrong with it anymore. Could work 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just needs to handle PermissionDenied
as discussed above.
Apparently this can occur on Windows.
OK |
This provides the same functionality but without temporary files, platform-specific code, fragility of
O_TMPFILE
support, and an extra dependency.