HackerTrans
TopNewTrendsCommentsPastAskShowJobs

joeshaw

no profile record

comments

joeshaw
·2 ปีที่แล้ว·discuss
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.
joeshaw
·2 ปีที่แล้ว·discuss
OP here. I appreciate the comments I've read here, and it might inspire a new blog post: "Defer is for resource cleanup, not error handling."
joeshaw
·2 ปีที่แล้ว·discuss
OP here. Another commenter pointed out that `errors.Join` didn't exist when I wrote this, but I wouldn't have changed my guidance if it had.

The core issue here is that you want to deal with errors as soon as possible. The async nature of writes to files makes this more challenging. Part of the purpose of this post was to inform people that you don't necessarily always see write errors at write-time, sometimes you see them at close-time and should handle them there. (And sometimes you don't see them at all without fsync.)

Even if you do handle errors in your deferred close, you may be taking other actions that you'd rather not have until the file i/o is completed, and this could leave your program in an inconsistent state. Side effects in Go are common, so this is a practical concern and it is hard to spot and debug.
joeshaw
·2 ปีที่แล้ว·discuss
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. (This makes it fine for `Close` on read-only file I/O operations, and not so great for writes.)