feat(local): explicit close with error checking for LocalFileSystem#676
Conversation
jgiannuzzi
changed the title
| 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()), | ||
| } |
There was a problem hiding this comment.
Since we already depend on nix, could we use that library on Unix instead?
There was a problem hiding this comment.
nix was only used as a dev dependency which is why I went with libc instead, cutting down the middleman (nix uses libc under the hood). I am happy to use nix instead though, as it simplifies this piece of code significantly.
There was a problem hiding this comment.
I think nix is a fine choice here and keeps the dev and prod world kinda in-sync 👍
| #[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)); | ||
| } |
There was a problem hiding this 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 pola-rs/polars#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?
There was a problem hiding this comment.
This test is just validating our logic by emulating a failure case.
The actual failure that can happen is when using a remote file system (e.g. NFS) and suffering from an underlying issue with the mount. In this case, the close could fail because some previous queued writes failed and checking the close return code allows the calling application to know that something failed, even if fsync wasn't called.
See this comment on the Polars issue for more details: pola-rs/polars#21002 (comment)
I don't think I claimed this was an alternative to fsync. It's rather a way to make sure these errors are reported to the caller. For durability guarantees, fsync remains required (and I think this should be part of another PR).
There was a problem hiding this comment.
Ah, NFS our old friend 😁 . This makes sense, thank you!
There was a problem hiding this comment.
BTW: you could also call close_file twice and expect that the 2nd call fails. That would make this test here also work on Windows.
There was a problem hiding this comment.
Good point! I've updated the Unix test and added a Windows version of it. Sadly I still need to use a raw fd/handle in order to not consume the file and be able to close it twice. Thank you Rust for generally preventing misusing APIs! (That's not sarcasm 😅)
jgiannuzzi
force-pushed
the
explicit-close-with-error-reporting
branch
from
dd6f9ff to
eee62ba
Compare
Add explicit close with error checking on Unix (libc::close) and Windows (CloseHandle), replacing silent drop.
jgiannuzzi
force-pushed
the
explicit-close-with-error-reporting
branch
from
eee62ba to
4a0ba0b
Compare
|
hey @crepererum, could you please take another look at this? |
crepererum
left a comment
crepererum
left a comment
There was a problem hiding this comment.
Looks good, thank you!
2 participants
Which issue does this PR close?
Implements one part of #661.
Rationale for this change
See issue. This PR is limited to explicitly calling
closeon written files with error checking.What changes are included in this PR?
close_file()helper that callslibc::close()(Unix) /CloseHandle(Windows) directly and propagates errors, instead of relying onFile::drop(). Note that this usesunsafe, like in the platforms' respectivedrop()implementations.putandput_multipart(viaLocalUpload) code paths.libc(Unix) andwindows-sys(Windows) as optional dependencies behind thefsfeature for their respective platforms.close_filedetectsEBADFwhen the file descriptor is closed behind Rust's back. This is just to validate thatclose_fileproperly returns the OS error.Are there any user-facing changes?
Close errors are now detected and propagated as
Error::UnableToCopyDataToFile. Previously these were silently ignored. This is a correctness improvement but could surface errors that were previously hidden.