diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index 888ca1af564..bf3ca3f8288 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -180,6 +180,19 @@ pub fn write, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> .with_context(|| format!("failed to write `{}`", path.display())) } +/// Writes a file to disk atomically. +/// +/// write_atomic uses tempfile::persist to accomplish atomic writes. +pub fn write_atomic, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> { + let path = path.as_ref(); + let mut tmp = TempFileBuilder::new() + .prefix(path.file_name().unwrap()) + .tempfile_in(path.parent().unwrap())?; + tmp.write_all(contents.as_ref())?; + tmp.persist(path)?; + Ok(()) +} + /// Equivalent to [`write()`], but does not write anything if the file contents /// are identical to the given contents. pub fn write_if_changed, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> { @@ -775,6 +788,29 @@ fn exclude_from_time_machine(path: &Path) { #[cfg(test)] mod tests { use super::join_paths; + use super::write; + use super::write_atomic; + + #[test] + fn write_works() { + let original_contents = "[dependencies]\nfoo = 0.1.0"; + + let tmpdir = tempfile::tempdir().unwrap(); + let path = tmpdir.path().join("Cargo.toml"); + write(&path, original_contents).unwrap(); + let contents = std::fs::read_to_string(&path).unwrap(); + assert_eq!(contents, original_contents); + } + #[test] + fn write_atomic_works() { + let original_contents = "[dependencies]\nfoo = 0.1.0"; + + let tmpdir = tempfile::tempdir().unwrap(); + let path = tmpdir.path().join("Cargo.toml"); + write_atomic(&path, original_contents).unwrap(); + let contents = std::fs::read_to_string(&path).unwrap(); + assert_eq!(contents, original_contents); + } #[test] fn join_paths_lists_paths_on_error() { diff --git a/src/bin/cargo/commands/remove.rs b/src/bin/cargo/commands/remove.rs index c6508a6b2d0..988c5fd21fe 100644 --- a/src/bin/cargo/commands/remove.rs +++ b/src/bin/cargo/commands/remove.rs @@ -270,7 +270,10 @@ fn gc_workspace(workspace: &Workspace<'_>) -> CargoResult<()> { } if is_modified { - cargo_util::paths::write(workspace.root_manifest(), manifest.to_string().as_bytes())?; + cargo_util::paths::write_atomic( + workspace.root_manifest(), + manifest.to_string().as_bytes(), + )?; } Ok(()) diff --git a/src/cargo/util/toml_mut/manifest.rs b/src/cargo/util/toml_mut/manifest.rs index 5529b802958..7d51f6b5c05 100644 --- a/src/cargo/util/toml_mut/manifest.rs +++ b/src/cargo/util/toml_mut/manifest.rs @@ -296,7 +296,7 @@ impl LocalManifest { let s = self.manifest.data.to_string(); let new_contents_bytes = s.as_bytes(); - cargo_util::paths::write(&self.path, new_contents_bytes) + cargo_util::paths::write_atomic(&self.path, new_contents_bytes) } /// Lookup a dependency. diff --git a/tests/testsuite/death.rs b/tests/testsuite/death.rs index f0e182d01a8..b61896dc917 100644 --- a/tests/testsuite/death.rs +++ b/tests/testsuite/death.rs @@ -1,12 +1,12 @@ //! Tests for ctrl-C handling. +use cargo_test_support::{project, slow_cpu_multiplier}; use std::fs; use std::io::{self, Read}; use std::net::TcpListener; use std::process::{Child, Stdio}; use std::thread; - -use cargo_test_support::{project, slow_cpu_multiplier}; +use std::time; #[cargo_test] fn ctrl_c_kills_everyone() { @@ -87,6 +87,155 @@ fn ctrl_c_kills_everyone() { ); } +#[cargo_test] +fn kill_cargo_add_never_corrupts_cargo_toml() { + cargo_test_support::registry::init(); + cargo_test_support::registry::Package::new("my-package", "0.1.1+my-package").publish(); + + let with_dependency = r#" +[package] +name = "foo" +version = "0.0.1" +authors = [] + +[dependencies] +my-package = "0.1.1" +"#; + let without_dependency = r#" +[package] +name = "foo" +version = "0.0.1" +authors = [] +"#; + + for sleep_time_ms in [30, 60, 90] { + let p = project() + .file("Cargo.toml", without_dependency) + .file("src/lib.rs", "") + .build(); + + let mut cargo = p.cargo("add").arg("my-package").build_command(); + cargo + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let mut child = cargo.spawn().unwrap(); + + thread::sleep(time::Duration::from_millis(sleep_time_ms)); + + assert!(child.kill().is_ok()); + assert!(child.wait().is_ok()); + + // check the Cargo.toml + let contents = fs::read(p.root().join("Cargo.toml")).unwrap(); + + // not empty + assert_ne!( + contents, b"", + "Cargo.toml is empty, and should not be at {} milliseconds", + sleep_time_ms + ); + + // We should have the original Cargo.toml or the new one, nothing else. + if std::str::from_utf8(&contents) + .unwrap() + .contains("[dependencies]") + { + assert_eq!( + std::str::from_utf8(&contents).unwrap(), + with_dependency, + "Cargo.toml is with_dependency after add at {} milliseconds", + sleep_time_ms + ); + } else { + assert_eq!( + std::str::from_utf8(&contents).unwrap(), + without_dependency, + "Cargo.toml is without_dependency after add at {} milliseconds", + sleep_time_ms + ); + } + } +} + +#[cargo_test] +fn kill_cargo_remove_never_corrupts_cargo_toml() { + let with_dependency = r#" +[package] +name = "foo" +version = "0.0.1" +authors = [] +build = "build.rs" + +[dependencies] +bar = "0.0.1" +"#; + let without_dependency = r#" +[package] +name = "foo" +version = "0.0.1" +authors = [] +build = "build.rs" +"#; + + // This test depends on killing the cargo process at the right time to cause a failed write. + // Note that we're iterating and using the index as time in ms to sleep before killing the cargo process. + // If it is working correctly, we never fail, but can't hang out here all day... + // So we'll just run it a few times and hope for the best. + for sleep_time_ms in [30, 60, 90] { + // new basic project with a single dependency + let p = project() + .file("Cargo.toml", with_dependency) + .file("src/lib.rs", "") + .build(); + + // run cargo remove the dependency + let mut cargo = p.cargo("remove").arg("bar").build_command(); + cargo + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + let mut child = cargo.spawn().unwrap(); + + thread::sleep(time::Duration::from_millis(sleep_time_ms)); + + assert!(child.kill().is_ok()); + assert!(child.wait().is_ok()); + + // check the Cargo.toml + let contents = fs::read(p.root().join("Cargo.toml")).unwrap(); + + // not empty + assert_ne!( + contents, b"", + "Cargo.toml is empty, and should not be at {} milliseconds", + sleep_time_ms + ); + + // We should have the original Cargo.toml or the new one, nothing else. + if std::str::from_utf8(&contents) + .unwrap() + .contains("[dependencies]") + { + assert_eq!( + std::str::from_utf8(&contents).unwrap(), + with_dependency, + "Cargo.toml is not the same as the original at {} milliseconds", + sleep_time_ms + ); + } else { + assert_eq!( + std::str::from_utf8(&contents).unwrap(), + without_dependency, + "Cargo.toml is not the same as expected at {} milliseconds", + sleep_time_ms + ); + } + } +} + #[cfg(unix)] pub fn ctrl_c(child: &mut Child) { let r = unsafe { libc::kill(-(child.id() as i32), libc::SIGINT) };