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

Use native fs.writeFile in lib/install/save.js #84

Closed
wants to merge 1 commit into from
Closed

Use native fs.writeFile in lib/install/save.js #84

wants to merge 1 commit into from

Conversation

corime
Copy link

@corime corime commented Oct 21, 2018

To write a file, write-file-atomic uses a temp file and replaces the original one by renaming the temp file to its original's name.
In some cases this produces an EBUSY error, if the original file cannot be removed/replaced (e.g. if the file, here package.json is mounted as a volume).
By using fs.writeFile this issue can be avoided, since it writes the given content directly into the original file and hence does not try to replace it.

Correct me if I'm wrong but I don't see a specific downside in using the native writeFile here.

To write a file, write-file-atomic uses a temp file and replaces the original one by renaming the temp file to its original's name. 
In some cases this produces an EBUSY error, if the original file cannot be removed/replaced.
By using fs.writeFile this issue can be avoided, since it writes the given content directly into the original file and hence does not try to replace it.

Correct me if I'm wrong but I don't think there is a specific downside in using the native writeFile here.
@corime corime requested a review from a team as a code owner October 21, 2018 12:14
@zkat
Copy link
Contributor

zkat commented Nov 13, 2018

There's a reason for doing this atomically, and that's to prevent awkward half-states in case of crashes. These are fairly precious files to be saving and we only want to finalize their write if everything is successful. If EBUSY errors are happening, that should be fixed, not circumvented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants