Add `close` method to `File`

[czipperz]

Currently, as far as I know, there is no way to close a file and handle errors with closing it. It would be useful to add a method to File that allows us to handle any potential errors instead of them being ignored.

fn close(self) -> std::io::Result<()>

[czipperz]

We could even add this method to Read and Write with a stub body returning Ok(()). Then File could override this in both cases to call it's own close method.

[czipperz]

Having three methods with the same declaration and name seems like bad design to me... Suggestions?

[czipperz]

Perhaps we should have a TryDrop trait that everything has a default implementation for?

pub auto trait TryDrop {
    type E = std::io::Error;
    fn try_drop(self) -> Result<(), E> { Ok(()) }
}

[czipperz]

However, this has major problems with recursive dropping. Like what if two different fields have different Es? I think making a separate trait out of it seems excessive.

Perhaps we should have a Close trait:

pub trait Close {
    fn close(self) -> Result<(), std::io::Error>;
}

Then have File implement it. BufReader and BufWriter implement it iff the underlying Read / Write instance supports it.

[Stargateur]

And what would be the benefice of having such error handling ? Error in closing file are generally logical error like trying to close a file not open (and this should happen in Rust). Also close a file doesn't guarantee that the file has been write too. So, I don't think we need this feature.

But I agree that not be able to handle this error is lame.

[tbu-]

And what would be the benefice of having such error handling ? Error in closing file are generally logical error like trying to close a file not open (and this should happen in Rust).

This is not true. According to the close man page, close can give you an error if the IO didn't finish successfully.

Also close a file doesn't guarantee that the file has been write too. So, I don't think we need this feature.

Yet the operating system can give us an error and we choose to ignore it.

[tbu-]

See also:

#24413 (reverse-duplicate of this bug)
#32255 (same)
#42619 (bug to document this issue)

rust-lang/rfcs#770 (closed RFC about this topic)
#22849 (PR implementing this and an unnecessary panic in the destructor)
https://github.com/rust-lang-nursery/cli-wg/issues/28 (someone else noticing this issue)
rust-lang/api-guidelines#61 (coming up with guidelines for possible close methods)

Some external discussions about the topic:
https://internals.rust-lang.org/t/fs-file-should-panic-if-implicit-close-fails-and-no-panic-is-active/1349?u=tbu
https://www.reddit.com/r/rust/comments/38ka6i/how_to_close_a_file/
https://www.reddit.com/r/rust/comments/5o8zk7/using_stdfsfile_as_shown_in_the_docs_can_lead_to/

[Stargateur]

@tbu- could != must

Not checking the return value of close() is a common but nevertheless serious programming error. It is quite possible that errors on a previous write(2) operation are first reported at the final close(). Not checking the return value when closing the file may lead to silent loss of data. This can especially be observed with NFS and with disk quota.

A successful close does not guarantee that the data has been successfully saved to disk, as the kernel defers writes. It is not common for a file system to flush the buffers when the stream is closed. If you need to be sure that the data is physically stored use fsync(2). (It will depend on the disk hardware at this point.)

https://linux.die.net/man/2/close

If any one need to be sure data is write, use sync_all is way better

[tbu-]

If any one need to be sure data is write, use sync_all is way better

Yes.

IMO, this doesn't mean we should throw away the error the operating system gives us.

[steveklabnik]

As @dtolnay says in #32255 (comment), this change would require an RFC. Please pursue this over at https://github.com/rust-lang/rfcs. Thanks!