Re: [PR] feat(local): explicit close with error checking for LocalFileSystem [arrow-rs-object-store]

["crepererum (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org