crepererum commented on code in PR #676:
URL:
https://github.com/apache/arrow-rs-object-store/pull/676#discussion_r3044274272
##########
src/local.rs:
##########
@@ -1913,4 +1956,16 @@ mod unix_test {
spawned.await.unwrap();
}
+
+ #[test]
+ fn test_close_file_detects_error() {
+ use std::os::unix::io::AsRawFd;
+
+ let file = tempfile::tempfile().unwrap();
+ let fd = file.as_raw_fd();
+ // Close the fd behind Rust's back so close_file will get EBADF
+ assert_eq!(unsafe { libc::close(fd) }, 0);
+ let err = super::close_file(file).unwrap_err();
+ assert_eq!(err.raw_os_error(), Some(libc::EBADF));
+ }
Review Comment:
This test tests the new function that you've just added. However I somewhat
fail to see what actual error case you're trying to prevent here. The file
descriptor isn't exposed to the API user of `object_store`. In #661 you
reference https://github.com/pola-rs/polars/issues/21002 but that issue is
about polar's own file handling, not about `object_store`. Also you say this is
an alternative to `fsync`, but that's not true. Closing the file doesn't give
you ANY guarantees with regards to durability. So what concrete order of events
leads to a case where the additional error check (via the manual FD close)
would help?
##########
src/local.rs:
##########
@@ -137,6 +137,37 @@ impl From<Error> for super::Error {
}
}
+/// Explicitly close a file, checking for errors that would be silently
ignored by Rust's `File::drop()`.
+///
+/// On network filesystems (e.g. NFS), `close()` can fail and indicate data
loss.
+fn close_file(file: File) -> std::result::Result<(), io::Error> {
+ #[cfg(target_family = "unix")]
+ {
+ use std::os::unix::io::IntoRawFd;
+ let fd = file.into_raw_fd();
+ // SAFETY: `fd` is a valid, owned file descriptor obtained from
`into_raw_fd()`.
+ match unsafe { libc::close(fd) } {
+ 0 => Ok(()),
+ _ => Err(io::Error::last_os_error()),
+ }
Review Comment:
Since we already depend on `nix`, could we use that library on Unix instead?
https://docs.rs/nix/latest/nix/unistd/fn.close.html
--
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]
