Perform atomic write to avoid corrupting files #12362
Conversation
this fixes issue where Ctrl+C (SIGINT or any other interruption) leaves files (like `Cargo.toml`) corrupted, and potentially empty
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
A-build-scripts
|
So @epage I'm getting this error in local testing when trying to copy from `Result::unwrap()` on an `Err` value: Os { code: 18, kind: CrossesDevices, message: "Invalid cross-device link" }'So I need a temporary file in the same filesystem? Where is a good place for that? EDIT: I guess we could pick |
| .write_all(contents.as_ref()) | ||
| .with_context(|| format!("failed to write to `{}`", temporary_file.path().display()))?; | ||
|
|
||
| fs::rename(temporary_file.path(), path) |
There was a problem hiding this comment.
So @epage I'm getting this error in local testing when trying to copy from
/tmp(generated bytempfilecrate) to current folder:`Result::unwrap()` on an `Err` value: Os { code: 18, kind: CrossesDevices, message: "Invalid cross-device link" }'So I need a temporary file in the same filesystem? Where is a good place for that?
EDIT: I guess we could pick
target/temporary_fileand we should be OK? File disappears after renaming anyways. EDIT2: nevermind,targetdoesn't exist aftercargo newandcargo add.
Atomic renames requires doing so on the same filesystem.
| pub fn atomic_write<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> { | ||
| let path = path.as_ref(); | ||
| fs::write(path, contents.as_ref()) | ||
| .with_context(|| format!("failed to write `{}`", path.display())) | ||
|
|
||
| let mut temporary_file = tempfile::Builder::new().suffix(path).tempfile()?; | ||
| temporary_file | ||
| .write_all(contents.as_ref()) | ||
| .with_context(|| format!("failed to write to `{}`", temporary_file.path().display()))?; | ||
|
|
||
| fs::rename(temporary_file.path(), path) |
There was a problem hiding this comment.
I know enough about atomic file writes to know that it is complicated. atomicwrites seems to be a popular crate so there might be things to learn from it (or maybe to use it).
Overall, I'd still want multiple eyes on this to make sure the approach is correct and its the right choice in each case.
There was a problem hiding this comment.
The bug is "wiped my file clean on ctrl-c" (#11386). I assume these "atomic writes" would instead be "I've got a random temp file on ctrl-c". Only slightly better. If that is the case, then I would expect this to only be applied to the cases where the empty file causes problems.
| /// are identical to the given contents. | ||
| pub fn write_if_changed<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> { | ||
| pub fn atomic_write_if_changed<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> { |
There was a problem hiding this comment.
This is a bit confusing of a name. Its "'atomic write' if changed" but could be read as "atomic 'write if changed'". which can't be atomic.
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
@marcospb19 do you plan on getting back to this? |
Not right now, feel free to pick it up if you want to. |
|
Thanks, and I hope you're doing well. |
|
Ty! Closing in favor of #12744. |
fix bug: corruption when cargo killed while writing ### What does this PR try to resolve? fix #11386, superseding #12362 ### How should we test and review this PR? Added unit test showing basic equivalency to existing `write(path, content)`. Full test suite should exercise write. Added tests for cargo add and remove. These are timing tests, so take a bit of time to run. 5-10s each. They may not fail every time, but do so regularly. Making the change to these two writes seems to prevent me from failing these tests at all. ### Additional information This uses tempfile::persist which was an existing dependency. atomicwrites crate, an alternative option for this fix, indicates `tempfile::persist` is the same thing. Since we already use tempfile as a dep, I stuck with that.
Fix #11386 with @epage's suggestion, atomic writes.
Instead of applying the fix just to
cargo-add, I applied it to everything, we might want to review each specific case to tell if cases.