reidkaufmann commented on code in PR #891:
URL:
https://github.com/apache/arrow-rs-object-store/pull/891#discussion_r4474556514
##########
src/local.rs:
##########
@@ -982,41 +985,66 @@ fn hard_link(original: &std::path::Path, link:
&std::path::Path, fsync: bool) ->
/// directory after — the rename, so a successful return is durable. The file
is always closed
/// before the rename (checking for close errors): required for NFS error
detection and for some
/// FUSE filesystems (e.g. Blobfuse) that only commit the data on close.
+///
+/// Returns the file's metadata. A failure before the rename succeeds removes
the staging file.
+/// After a successful rename its path may already belong to a new upload, so
it is left alone.
fn finish_staged_rename(
file: File,
src: &std::path::Path,
dest: &std::path::Path,
fsync: bool,
-) -> Result<()> {
- sync_and_close(file, src, fsync)?;
- rename(src, dest, fsync).map_err(|source| Error::UnableToRenameFile {
source })?;
- Ok(())
+) -> Result<Metadata> {
+ let closed = staged_metadata(&file, src).and_then(|metadata| {
+ sync_and_close(file, src, fsync)?;
+ Ok(metadata)
+ });
+ let err = match closed {
+ Ok(metadata) => match rename(src, dest, fsync) {
+ Ok(()) => return Ok(metadata),
+ Err(RenameError::Fsync(source)) => {
+ return Err(Error::UnableToRenameFile { source }.into());
+ }
+ Err(RenameError::Rename(source)) => Error::UnableToRenameFile {
source }.into(),
+ },
+ Err(e) => e,
+ };
+ let _ = std::fs::remove_file(src); // Attempt to cleanup
+ Err(err)
Review Comment:
`file` is moved into the `and_then` closure, so when `staged_metadata` fails
the closure (and the handle) is dropped before `remove_file` runs.
##########
src/local.rs:
##########
@@ -654,7 +639,11 @@ impl ObjectStore for LocalFileSystem {
Ok(_) => match rename(&staged, &to, fsync) {
Ok(_) => return Ok(()),
Err(source) => {
- let _ = std::fs::remove_file(&staged); //
Attempt to clean up
+ // Once renamed, `staged` may already be a
new upload's file.
+ if let RenameError::Rename(_) = source {
+ let _ = std::fs::remove_file(&staged);
// Attempt to clean up
Review Comment:
Exercising this needs another upload to take the staging name between the
rename and the cleanup, which I can't do deterministically without a test hook
in production code; the arm is a two-line match on `RenameError::Rename`.
##########
src/local.rs:
##########
@@ -982,41 +985,66 @@ fn hard_link(original: &std::path::Path, link:
&std::path::Path, fsync: bool) ->
/// directory after — the rename, so a successful return is durable. The file
is always closed
/// before the rename (checking for close errors): required for NFS error
detection and for some
/// FUSE filesystems (e.g. Blobfuse) that only commit the data on close.
+///
+/// Returns the file's metadata. A failure before the rename succeeds removes
the staging file.
+/// After a successful rename its path may already belong to a new upload, so
it is left alone.
fn finish_staged_rename(
file: File,
src: &std::path::Path,
dest: &std::path::Path,
fsync: bool,
-) -> Result<()> {
- sync_and_close(file, src, fsync)?;
- rename(src, dest, fsync).map_err(|source| Error::UnableToRenameFile {
source })?;
- Ok(())
+) -> Result<Metadata> {
+ let closed = staged_metadata(&file, src).and_then(|metadata| {
+ sync_and_close(file, src, fsync)?;
+ Ok(metadata)
+ });
+ let err = match closed {
+ Ok(metadata) => match rename(src, dest, fsync) {
+ Ok(()) => return Ok(metadata),
+ Err(RenameError::Fsync(source)) => {
+ return Err(Error::UnableToRenameFile { source }.into());
+ }
+ Err(RenameError::Rename(source)) => Error::UnableToRenameFile {
source }.into(),
+ },
+ Err(e) => e,
+ };
+ let _ = std::fs::remove_file(src); // Attempt to cleanup
+ Err(err)
}
/// Like [`finish_staged_rename`] but publishes via a hard link
(`PutMode::Create` semantics): the
-/// staging file is linked to `dest` and then removed. Returns
[`Error::AlreadyExists`] if `dest`
-/// already exists.
+/// staging file is linked to `dest` and then removed, whether or not linking
succeeds. Returns
+/// [`Error::AlreadyExists`] if `dest` already exists.
fn finish_staged_hard_link(
file: File,
src: &std::path::Path,
dest: &std::path::Path,
fsync: bool,
-) -> Result<()> {
- sync_and_close(file, src, fsync)?;
- match hard_link(src, dest, fsync) {
- Ok(()) => {
- let _ = std::fs::remove_file(src); // Attempt to cleanup
- Ok(())
- }
- Err(source) => match source.kind() {
- ErrorKind::AlreadyExists => Err(Error::AlreadyExists {
+) -> Result<Metadata> {
+ let linked = staged_metadata(&file, src).and_then(|metadata| {
+ sync_and_close(file, src, fsync)?;
+ hard_link(src, dest, fsync).map_err(|source| match source.kind() {
+ ErrorKind::AlreadyExists => Error::AlreadyExists {
path: dest.to_str().unwrap().to_string(),
source,
- }
- .into()),
- _ => Err(Error::UnableToRenameFile { source }.into()),
- },
- }
+ },
+ _ => Error::UnableToRenameFile { source },
+ })?;
Review Comment:
This `unwrap` is unchanged from `main` (moved here); I'd keep it out of this
PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
