Live data from Hacker News

Don't defer Close() on writable files (2017)

joeshaw.org

201–210 of 311 posts

Re: Don't defer Close() on writable files (2017)

#201
post #134

Earlier quoted context omitted.

Phew. I've primarily used Windows. Not that any of the posix programs I've been exposed to have done the dir sync though. For cross-platform stuff I've mainly used Boost, which I assumed handled such details.

I'm sure it does not. Also these things are needed very very rarely (which is why few even know about the issue) and are not good for performance and battery life.

More specifically, these things are for trying to improve the behavior on unclean system shutdown (e.g. power loss) which is inherently chaotic and unless all parts (most critically the disk and its controller) are well behaved you don't have any real guarantees anyway.

Windows also doesn't guarantee that data is written to disk by the time WriteFile/CloseHandle returns, the Windows version of fsync is FlushFileBuffers.

Re: Don't defer Close() on writable files (2017)

#202

Earlier quoted context omitted.

"if you are creating a file, to ensure full synchronisation you also need to fsync the parent directory, otherwise the file can be fsynced but the update to the directory lost." And if you need this in Java you still have resort to ugly hacks. https://github.com/apache/lucene/issues/7231

This says the bug is fixed? https://bugs.openjdk.org/browse/JDK-8066915

The point is that there is no official way to fsync a directory in Java and that everyone is relying on an unintentional side effect of an unrelated function to accomplish it. The link I supplied is about the fact that the side effect briefly disappeared in Java 9 until enough people complained.

We're still living in xkcd 1172 land with this, have been for a decade or who knows how long.

Re: Don't defer Close() on writable files (2017)

#203
post #152

Earlier quoted context omitted.

So if I do fopen/fwrite/fsync/fclose, that is not enough? That is crazy, I think 90% of apps don't fsync the parent directory. Also, how many levels of parents do you need to fsync?

You don't need even to bother with fsync unless you are developing a database or use the file system like a database. There's a reason Apple made fsync useless and introduced F_FULLSYNC. They know developers have an incentive to overestimate the importance of their own files at the detriment to system responsiveness and power draw.

Agreed, things like Firefox constantly fsyncing its profile database is absurd.

Re: Don't defer Close() on writable files (2017)

#204
post #76

Earlier quoted context omitted.

I caution against this approach, as you are not really dealing with the error when it occurs. If the work you do after the defer has other side effects, you may have just gotten your application into an inconsistent state and it's very hard to see in code why this might be. `defer` is really not well-suited for error handling, its benefit is mainly in resource cleanup where failure is impossible or doesn't matter. (T…

> If the work you do after the defer has other side effects, you may have just gotten your application into an inconsistent state and it's very hard to see in code why this might be. Can you give an example case of how this could happen?

This is a contrived example, but imagine a situation where I have a file I want to write on disk and then have a reference to it in a database. If I have a flow like:

    func UpdateUser(user *User) (err error) {
        f, err := os.Create("/some/file.txt")
        if err != nil {
            return err
        }
        defer CheckClose(f, &err)

        if _, err := f.Write(somedata); err != nil {
            return err
        }

        if err := db.UpdateUser(user, "/some/file.txt"); err != nil {
            return err
        }

        return
    }

This function might have updated the user in the database with a new file despite the fact that `CheckClose` (defined up-thread) does check to see if the `Close` failed and returned an error. The calling code won't have known this has happened.

The core problem is that the error checking is not done soon enough, either because Go programmers are conditioned to `defer f.Close()` from nearly all example code -- most of it demonstrating reads, not writes -- or because they are handling the error, but only in a deferred function, not earlier.

A more correct way to do this would be:

    func UpdateUser(user *User) error {
        f, err := os.Create("/some/file.txt")
        if err != nil {
            return err
        }
        defer f.Close()

        if _, err := f.Write(somedata); err != nil {
            return err
        }

        if err := f.Sync(); err != nil {
            return err
        }

        if err := f.Close(); err != nil {
            return err
        }

        if err := db.UpdateUser(user, "/some/file.txt"); err != nil {
            return err
        }
    }
`Sync()` flushes the data to disk, and `Close()` gives a "last-chance" opportunity to return an error. The `defer f.Close()` exists as a way to ensure resource cleanup if an error occurs before the explicit `f.Close()` toward the end of the function. As I mentioned in an update to the post, double `Close()` is fine.

Re: Don't defer Close() on writable files (2017)

#205
post #172

Boggles my mind that after more than 60 years of computer science, we still design tools (programming languages) where the simplest tasks are full of gotchas and footguns. This is a great example.

It's hardly a footgun. Close may be able to report some additional errors with getting the data on persistent storage but it won't report all of them anyway. For most applications, ignoring the return of close is perfectly fine in practice.

Re: Don't defer Close() on writable files (2017)

#206

Earlier quoted context omitted.

If I understand the OP correctly, if Close() fails then you can't trust that the data was written, even if the previous Write() succeeded.

Exactly that; for critical operations like e.g. a database, if a write fails you've got corrupted data and you have a Major Issue. That said, I'm not sure how they would handle a file close failure, wouldn't the file be corrupted anyway because some of the bits may have been written? Then again, at least you can raise the alarms if Close fails, because silent failures are worse than failures.

> if a write fails.

We're talking about a read-only case. os.Open returns a read-only file handle. If you try writing to it, you'll get an error already at that point. If close fails, who cares?

> I'm not sure how they would handle a file close failure

Ideally there is some kind of failover you can resort to, but if there is no other option at very least you will want to notify a human that what they thought was written isn't actually. But when only reading, you don't need to fall back to anything – all the reads were successful – and what is it to the human? What they thought was supposed to happen did!

Re: Don't defer Close() on writable files (2017)

#207
On production systems, it may often be better to completely ignore the problem at this level. On modern hardware if a disk is throwing an io error on write you are having a bad day. I can almost guarantee that while you might happily modify your code so it properly returns the error, there almost certainly aren't test cases for ensuring such situations are handled "correctly", especially since the error will almost certainly not occur in isolation.

It may often be better to handle the issue as a system failure with fanotify. https://docs.kernel.org/admin-guide/filesystem-monitoring.ht...

Re: Don't defer Close() on writable files (2017)

#208
post #7

There are more wrinkles with this: - if you are creating a file, to ensure full synchronisation you also need to fsync the parent directory, otherwise the file can be fsynced but the update to the directory lost - if sync fails, you can not assume anything about the file, whether on-disk or in memory , critically one understanding which got dubbed "fsyncgate" and lead to many RDBMS having to be updated is that you ca…

More on fsyncgate: https://lwn.net/Articles/752063/

Re: Don't defer Close() on writable files (2017)

#209

how, when and why can close() fail? and what can you do about it if it does?

Fails if (say) OS can't write out pending cache and confirm data written to device. Causes include memory failure, drive cable melted, network cable pulled, etc. What to do? How important is the data being written? Is the only copy of just aquired data from a $10 million day geophysical survey? How much time and resources can you spend on work arounds, multiple copies, alternative storage paths, etc. In aquisition yo…

Handling it this way in a user process is insane and essentially cargo culting. If your data is that valuable, you have redundant systems.

Re: Don't defer Close() on writable files (2017)

#210
post #172

Boggles my mind that after more than 60 years of computer science, we still design tools (programming languages) where the simplest tasks are full of gotchas and footguns. This is a great example.

It's hardly a footgun. Close may be able to report some additional errors with getting the data on persistent storage but it won't report all of them anyway. For most applications, ignoring the return of close is perfectly fine in practice.

Agreed. Just log it and move on. The code _probably_ wrote what it needed to even if it didn't close. If truly cared that you got everything out correctly, you'd need to do more work than a blind `defer Close()` anyway and you'd never have written the code like this.
Post reply on HN