From 20387f24aada21b46a56e437ac9f2d495cfa30dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E5=B0=8F=E7=99=BD?= <364772080@qq.com> Date: Thu, 8 May 2025 19:57:16 +0800 Subject: [PATCH] windows: Fix atomic write (#30234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Superseded #30222 On Windows, `MoveFileExW` fails if another process is holding a handle to the file. This PR fixes that issue by switching to `ReplaceFileW` instead. I’ve also added corresponding tests. According to [this Microsoft research paper](https://www.microsoft.com/en-us/research/wp-content/uploads/2006/04/tr-2006-45.pdf) and the [official documentation](https://learn.microsoft.com/en-us/windows/win32/fileio/deprecation-of-txf#applications-updating-a-single-file-with-document-like-data), `ReplaceFileW` is considered an atomic operation. even though the official docs don’t explicitly state whether `MoveFileExW` or `ReplaceFileW` is guaranteed to be atomic. Release Notes: - N/A --- crates/fs/src/fs.rs | 123 +++++++++++++++++++++++++++++++++----------- 1 file changed, 92 insertions(+), 31 deletions(-) diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index d1c59a2b58..e2721acda6 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -33,7 +33,7 @@ use std::{ sync::Arc, time::{Duration, SystemTime, UNIX_EPOCH}, }; -use tempfile::{NamedTempFile, TempDir}; +use tempfile::TempDir; use text::LineEnding; #[cfg(any(test, feature = "test-support"))] @@ -525,45 +525,19 @@ impl Fs for RealFs { Ok(bytes) } + #[cfg(not(target_os = "windows"))] async fn atomic_write(&self, path: PathBuf, data: String) -> Result<()> { smol::unblock(move || { let mut tmp_file = if cfg!(any(target_os = "linux", target_os = "freebsd")) { // Use the directory of the destination as temp dir to avoid // invalid cross-device link error, and XDG_CACHE_DIR for fallback. // See https://github.com/zed-industries/zed/pull/8437 for more details. - NamedTempFile::new_in(path.parent().unwrap_or(paths::temp_dir())) - } else if cfg!(target_os = "windows") { - // If temp dir is set to a different drive than the destination, - // we receive error: - // - // failed to persist temporary file: - // The system cannot move the file to a different disk drive. (os error 17) - // - // So we use the directory of the destination as a temp dir to avoid it. - // https://github.com/zed-industries/zed/issues/16571 - NamedTempFile::new_in(path.parent().unwrap_or(paths::temp_dir())) + tempfile::NamedTempFile::new_in(path.parent().unwrap_or(paths::temp_dir())) } else { - NamedTempFile::new() + tempfile::NamedTempFile::new() }?; tmp_file.write_all(data.as_bytes())?; - - let result = tmp_file.persist(&path); - if cfg!(target_os = "windows") { - // If file handle is already in used we receive error: - // - // failed to persist temporary file: - // Access is denied. (os error 5) - // - // So we use direct fs write instead to avoid it. - // https://github.com/zed-industries/zed/issues/30054 - if let Err(persist_err) = &result { - if persist_err.error.raw_os_error() == Some(5) { - return std::fs::write(&path, data.as_bytes()).map_err(Into::into); - } - } - } - result?; - + tmp_file.persist(path)?; Ok::<(), anyhow::Error>(()) }) .await?; @@ -571,6 +545,35 @@ impl Fs for RealFs { Ok(()) } + #[cfg(target_os = "windows")] + async fn atomic_write(&self, path: PathBuf, data: String) -> Result<()> { + smol::unblock(move || { + // If temp dir is set to a different drive than the destination, + // we receive error: + // + // failed to persist temporary file: + // The system cannot move the file to a different disk drive. (os error 17) + // + // This is because `ReplaceFileW` does not support cross volume moves. + // See the remark section: "The backup file, replaced file, and replacement file must all reside on the same volume." + // https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-replacefilew#remarks + // + // So we use the directory of the destination as a temp dir to avoid it. + // https://github.com/zed-industries/zed/issues/16571 + let temp_dir = TempDir::new_in(path.parent().unwrap_or(paths::temp_dir()))?; + let temp_file = { + let temp_file_path = temp_dir.path().join("temp_file"); + let mut file = std::fs::File::create_new(&temp_file_path)?; + file.write_all(data.as_bytes())?; + temp_file_path + }; + atomic_replace(path.as_path(), temp_file.as_path())?; + Ok::<(), anyhow::Error>(()) + }) + .await?; + Ok(()) + } + async fn save(&self, path: &Path, text: &Rope, line_ending: LineEnding) -> Result<()> { let buffer_size = text.summary().len.min(10 * 1024); if let Some(path) = path.parent() { @@ -2486,6 +2489,31 @@ async fn file_id(path: impl AsRef) -> Result { .await } +#[cfg(target_os = "windows")] +fn atomic_replace>( + replaced_file: P, + replacement_file: P, +) -> windows::core::Result<()> { + use windows::{ + Win32::Storage::FileSystem::{REPLACE_FILE_FLAGS, ReplaceFileW}, + core::HSTRING, + }; + + // If the file does not exist, create it. + let _ = std::fs::File::create_new(replaced_file.as_ref()); + + unsafe { + ReplaceFileW( + &HSTRING::from(replaced_file.as_ref().to_string_lossy().to_string()), + &HSTRING::from(replacement_file.as_ref().to_string_lossy().to_string()), + None, + REPLACE_FILE_FLAGS::default(), + None, + None, + ) + } +} + #[cfg(test)] mod tests { use super::*; @@ -2905,4 +2933,37 @@ mod tests { "B" ); } + + #[gpui::test] + async fn test_realfs_atomic_write(executor: BackgroundExecutor) { + // With the file handle still open, the file should be replaced + // https://github.com/zed-industries/zed/issues/30054 + let fs = RealFs { + git_binary_path: None, + executor, + }; + let temp_dir = TempDir::new().unwrap(); + let file_to_be_replaced = temp_dir.path().join("file.txt"); + let mut file = std::fs::File::create_new(&file_to_be_replaced).unwrap(); + file.write_all(b"Hello").unwrap(); + // drop(file); // We still hold the file handle here + let content = std::fs::read_to_string(&file_to_be_replaced).unwrap(); + assert_eq!(content, "Hello"); + smol::block_on(fs.atomic_write(file_to_be_replaced.clone(), "World".into())).unwrap(); + let content = std::fs::read_to_string(&file_to_be_replaced).unwrap(); + assert_eq!(content, "World"); + } + + #[gpui::test] + async fn test_realfs_atomic_write_non_existing_file(executor: BackgroundExecutor) { + let fs = RealFs { + git_binary_path: None, + executor, + }; + let temp_dir = TempDir::new().unwrap(); + let file_to_be_replaced = temp_dir.path().join("file.txt"); + smol::block_on(fs.atomic_write(file_to_be_replaced.clone(), "Hello".into())).unwrap(); + let content = std::fs::read_to_string(&file_to_be_replaced).unwrap(); + assert_eq!(content, "Hello"); + } }