FileDesc::drop leaks file descriptors and has useless error reporting

[mahkoh]

impl Drop for FileDesc {
    fn drop(&mut self) {
        // closing stdio file handles makes no sense, so never do it. Also, note
        // that errors are ignored when closing a file descriptor. The reason
        // for this is that if an error occurs we don't actually know if the
        // file descriptor was closed or not, and if we retried (for something
        // like EINTR), we might close another valid file descriptor (opened
        // after we closed ours.
        if self.close_on_drop && self.fd > libc::STDERR_FILENO {
            let n = unsafe { libc::close(self.fd) };
            if n != 0 {
                println!("error {} when closing file descriptor {}", n, self.fd);
            }
        }
    }
}

[mahkoh]

And even if the error reporting were better, it should not print to stdout.

[thestinger]

It shouldn't really do anything if close fails there. There are very few errors it will report anyway, since it doesn't block until writes are done. The caller needs to use fsync if they care.

[nodakai]

Shouldn't drop() panic upon catching the error of close(2)? Because it implies there's an error in the application logic from which we have no way to recover. I think this is what panicking is for.

Comparing the file descriptor value with libc::STDERR_FILENO, i.e. 2 is not very meaningful, as shown by this C code:

#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>

int main(void) {
    int fd;

    fd = open("/", 0);
    printf("%d\n", fd); /* => 3 */
    close(fd);

    fclose(stdin);

    fd = open("/", 0);
    printf("%d\n", fd); /* => 0 */
    close(fd);

    return 0;
}

closing stdio file handles makes no sense

Why?

[thestinger]

@nodakai: It's not sane to panic in a destructor. An EIO error from close does not indicate a bug in the application and it shouldn't bring it down. The caller can check for errors with fsync if they need a guarantee that the operation succeeded. The close call does not block until it is flushed.

[thestinger]

Panic in a destructor will cause resource leaks in other types, along with bringing down the entire task and aborting the process if it was already unwinding. It doesn't ever make sense to do it deliberately.

[nodakai]

@thestinger Hmm, points taken; panicking in the middle of stack unwinding is problematic, at least as of now (I hope we can devise a saner semantics than C++ for it...) And when I wrote "logic error in an application," I had only EBADF in my mind.

The close call does not block until it is flushed.

I assume you meant "does not return". Right, Linux NFS, among others, does synchronization upon closing an fd. Then EINTR and EIO are no surprise.

The caller can check for errors with fsync if they need a guarantee that the operation succeeded.

I don't quite understand why you mentioned fsync(2) here. HW-level data integrity and behavior of syscalls (what did you mean by "the operation"?) are two different things. Please also remember neither fsync(2) nor fdatasync(2) work with pipes:

#define _POSIX_C_SOURCE 200112L
#include <stdio.h>
#include <unistd.h>

int main(void) {
    int fds[2];
    if (pipe(fds)) perror("pipe(2)");
    if (fsync(fds[0])) perror("fsync(2)");
    if (fsync(fds[1])) perror("fsync(2)");
    if (fdatasync(fds[0])) perror("fdatasync(2)");
    if (fdatasync(fds[1])) perror("fdatasync(2)");
    return 0;
}

fsync(2): Invalid argument
fsync(2): Invalid argument
fdatasync(2): Invalid argument
fdatasync(2): Invalid argument

[mahkoh]

Still broken.

[alexcrichton]

Changed in eadc3bc