From 9245061fa1868ea4ec55351ccff0fb0d0b354f5d Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 16 Dec 2023 17:20:48 +0900 Subject: [PATCH 1/3] file_util: drop open file instance from PersistError PersistError is basically a pair of io::Error and NamedTempFile instance. It's unlikely that we would want to propagate the open file instance to the CLI error handler, leaving the temporary file alive. --- lib/src/file_util.rs | 6 +++--- lib/src/simple_op_store.rs | 14 +++++--------- lib/src/stacked_table.rs | 5 +---- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/lib/src/file_util.rs b/lib/src/file_util.rs index 56cdbd9b56..e32f878414 100644 --- a/lib/src/file_util.rs +++ b/lib/src/file_util.rs @@ -95,14 +95,14 @@ pub fn normalize_path(path: &Path) -> PathBuf { pub fn persist_content_addressed_temp_file>( temp_file: NamedTempFile, new_path: P, -) -> Result { +) -> io::Result { match temp_file.persist(&new_path) { Ok(file) => Ok(file), - Err(PersistError { error, file }) => { + Err(PersistError { error, file: _ }) => { if let Ok(existing_file) = File::open(new_path) { Ok(existing_file) } else { - Err(PersistError { error, file }) + Err(error) } } } diff --git a/lib/src/simple_op_store.rs b/lib/src/simple_op_store.rs index 4530989893..88f1f775e7 100644 --- a/lib/src/simple_op_store.rs +++ b/lib/src/simple_op_store.rs @@ -21,7 +21,7 @@ use std::io::{ErrorKind, Write}; use std::path::{Path, PathBuf}; use prost::Message; -use tempfile::{NamedTempFile, PersistError}; +use tempfile::NamedTempFile; use thiserror::Error; use crate::backend::{CommitId, MillisSinceEpoch, ObjectId, Timestamp}; @@ -34,12 +34,6 @@ use crate::op_store::{ }; use crate::{git, op_store}; -impl From for OpStoreError { - fn from(err: PersistError) -> Self { - OpStoreError::Other(err.into()) - } -} - #[derive(Debug, Error)] #[error("Failed to read {kind} with ID {id}: {err}")] struct DecodeError { @@ -119,7 +113,8 @@ impl OpStore for SimpleOpStore { let id = ViewId::new(blake2b_hash(view).to_vec()); - persist_content_addressed_temp_file(temp_file, self.view_path(&id))?; + persist_content_addressed_temp_file(temp_file, self.view_path(&id)) + .map_err(|err| io_to_write_error(err, "view"))?; Ok(id) } @@ -148,7 +143,8 @@ impl OpStore for SimpleOpStore { let id = OperationId::new(blake2b_hash(operation).to_vec()); - persist_content_addressed_temp_file(temp_file, self.operation_path(&id))?; + persist_content_addressed_temp_file(temp_file, self.operation_path(&id)) + .map_err(|err| io_to_write_error(err, "operation"))?; Ok(id) } } diff --git a/lib/src/stacked_table.rs b/lib/src/stacked_table.rs index 75c9319f09..1cc91cbc1f 100644 --- a/lib/src/stacked_table.rs +++ b/lib/src/stacked_table.rs @@ -359,10 +359,7 @@ impl TableSegment for MutableTable { #[derive(Debug, Error)] #[error(transparent)] -pub enum TableStoreError { - IoError(#[from] io::Error), - PersistError(#[from] tempfile::PersistError), -} +pub struct TableStoreError(#[from] pub io::Error); pub type TableStoreResult = Result; From d8d5fce3c423a2f5f11be143e62265fbdd027ebd Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 16 Dec 2023 16:47:34 +0900 Subject: [PATCH 2/3] file_util: don't try to overwrite existing content-addressed file on Windows The doc says persist() replaces the destination file as rename() would do on Unix. persist_noclobber() doesn't, and is probably more reliable on Windows. I don't know if persist() is completely atomic on Windows, but if it isn't, it might be the source of the "permission denied" error under highly contended situation. https://docs.rs/tempfile/latest/tempfile/struct.NamedTempFile.html#method.persist https://github.com/Stebalien/tempfile/blob/v3.8.0/src/file/imp/windows.rs#L77 We could use persist_noclobber() on all platforms, but it's more involved on Unix. https://github.com/Stebalien/tempfile/blob/v3.8.0/src/file/imp/unix.rs#L107 --- lib/src/file_util.rs | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/lib/src/file_util.rs b/lib/src/file_util.rs index e32f878414..4a45b93463 100644 --- a/lib/src/file_util.rs +++ b/lib/src/file_util.rs @@ -90,21 +90,33 @@ pub fn normalize_path(path: &Path) -> PathBuf { } } -// Like NamedTempFile::persist(), but also succeeds if the target already -// exists. +/// Like `NamedTempFile::persist()`, but doesn't try to overwrite the existing +/// target on Windows. pub fn persist_content_addressed_temp_file>( temp_file: NamedTempFile, new_path: P, ) -> io::Result { - match temp_file.persist(&new_path) { - Ok(file) => Ok(file), - Err(PersistError { error, file: _ }) => { - if let Ok(existing_file) = File::open(new_path) { - Ok(existing_file) - } else { - Err(error) + if cfg!(windows) { + // On Windows, overwriting file can fail if the file is opened without + // FILE_SHARE_DELETE for example. We don't need to take a risk if the + // file already exists. + match temp_file.persist_noclobber(&new_path) { + Ok(file) => Ok(file), + Err(PersistError { error, file: _ }) => { + if let Ok(existing_file) = File::open(new_path) { + Ok(existing_file) + } else { + Err(error) + } } } + } else { + // On Unix, rename() is atomic and should succeed even if the + // destination file exists. Checking if the target exists might involve + // non-atomic operation, so don't use persist_noclobber(). + temp_file + .persist(new_path) + .map_err(|PersistError { error, file: _ }| error) } } From e575f7f64cd9542cd90df0549e6eb801f1ae0455 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 16 Dec 2023 17:40:49 +0900 Subject: [PATCH 3/3] working_copy: drop open file instance from PersistError For the same reason as the file_util change. --- lib/src/local_working_copy.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 53135e5dcf..13fa2be7fd 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -494,7 +494,7 @@ pub enum TreeStateError { #[error("Persisting tree state to file {path}: {source}")] PersistTreeState { path: PathBuf, - source: tempfile::PersistError, + source: std::io::Error, }, #[error("Filesystem monitor error: {0}")] Fsmonitor(Box), @@ -646,9 +646,11 @@ impl TreeState { let target_path = self.state_path.join("tree_state"); temp_file .persist(&target_path) - .map_err(|err| TreeStateError::PersistTreeState { - path: target_path.clone(), - source: err, + .map_err(|tempfile::PersistError { error, file: _ }| { + TreeStateError::PersistTreeState { + path: target_path.clone(), + source: error, + } })?; Ok(()) }