Hacker News Reader

Comment by nickcw | Hacker News Reader

Here is my favorite solution to this problem

  // CheckClose is a utility function used to check the return from
  // Close in a defer statement.
  func CheckClose(c io.Closer, err *error) {
          cerr := c.Close()
          if *err == nil {
                  *err = cerr
          }
  }

Use like this - you must name the error return

  func whatever() (err error) {
      f, err := os.Open(blah)
      // ...
      defer CheckClose(f, &err)
      // ...
  }

This closes the file and if there wasn't an existing error, writes the error from f.Close() in there.

View on HN

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.)

> If the work you do after the defer has other side effects

Defer is by definition the last work you do in a function, there won't be more work except by the caller who will get the error returned to them.

If you are structuring a function that writes a file, and then does something with it, defer isn't appropriate, since you should close it before you do any more work.

It's possible to have multiple defers in a function though (so you have multiple "last work in a function"; nowhere is it dictated that a function should only have one operation that needs to clean something up at the end. Think for example copying one file to another.

> 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.

I think a better solution is to write smaller, single-purpose functions. To refer to your example downthread, you should have one function that only writes the file, and another that does the "whole" operation -- calling the function to write the file, checking for errors, and then updating the database.

Then you can use defer in the file-writing function if you so please, and not bother to close at the end explicitly, without issue. A more robust example might be to even include the sync call in the deferred function (and even clean up the file itself on error). To re-use your example from your blog post:.

    func helloNotes() (err error) {
        var f *os.File
        f, err = os.Create("/home/joeshaw/notes.txt")
        if err != nil {
            return
        }

        defer func() {
            if err == nil {
                err = f.Sync()
            }

            cerr := f.Close()
            if err == nil {
                err = cerr
            }

            if err != nil {
                os.Remove("/home/joeshaw/notes.txt")
            }
        }()

        err = io.WriteString(f, "hello world")
        return
    }

I would probably move that out into a helper, though, so I could do something like

    defer SafeClose(f, &err)

instead, and be able to use it elsewhere. Hell, even without defer, it's nice to have a helper that will sync and close for you so you can avoid the boilerplate, if you have lots of different bits of code that writes files.

FWIW, I'm not sure why you are so negative on named return values, but I'm at best a novice Go programmer, so perhaps I don't fully understand why they aren't great (I guess it does look weird to me to have bare `return` statements that do actually return a value even though it doesn't look like it). Your argument about the return value possibly being modified after the core function finishes being unintuitive doesn't really strike me as a big deal either.

I use a similar pattern, cribbed from one of the Go databases that aspired never to ignore errors. The difference is that errs.Capture preserves existing errors and formats the error with a message--important if generalized to handle any error function.

    package errs

    // Capture runs errFunc and assigns the error, if any, to *errPtr. Preserves the
    // original error by wrapping with errors.Join if the errFunc err is non-nil.
    func Capture(errPtr *error, errFunc func() error, msg string) {
        err := errFunc()
        if err == nil {
            return
        }
        *errPtr = errors.Join(*errPtr, fmt.Errorf("%s: %w", msg, err))
    }

I conventionally use mErr to distinguish from err.

    func doThing() (_ string, mErr error) {
        f, err := os.Open("foo")
        if err != nil {
            return "", fmt.Errorf("open file: %w", err)
        }
        // Use the file...
        defer errs.Capture(&mErr, f.Close, "close file")

        return "", nil
    }

I still question why defer doesn't support doing exactly that.

After all it's like the go language provide us with a cleanup function that in 99% of the time shouldn't be used unless we manually wrap what it's calling to properly handle error.

In the end, what's the point of defer ?

> I still question why defer doesn't support doing exactly that.

When would it ever be useful? You'd soon start to hate life if you actually tried using the above function in anything beyond a toy application.

> 99% of the time shouldn't be used

1. 99% of the time it is fine to use without further consideration. Even if there are errors, they don't matter. The example from the parent comment is a perfect case in point. Who cares if Close fails? It doesn't affect you in any way.

2. 0.999% of the time if you have a function that combines an operation that might fail in a manner you need to deal with along with cleanup it will be designed to allow being called more than once, allowing you, the caller, to separate the operation and cleanup phases in your code.

3. 0.001% you might have to be careful about its use if a package has an ill-conceived API. If you can, fix the API. The chances of you encountering this is slim, though, especially if you don't randomly import packages written by a high school student writing code for the first time ever.

The whole point of this post is that an error returned from file.Close DOES matter

I think from a Go point of view, the lesson to be drawn from that is "don't defer a function call if you need to check its error value", rather than "defer needs to support checking of function return values".

In the example at hand, it really makes more sense to call Close() as soon as possible after the file is written. It's more of an issue with the underlying OS file API making error checking difficult.

In 99% of cases, the solution to this problem will be to use a WriteFile function that opens, writes and closes the file and does all the error handling for you.

> the lesson to be drawn from that is "don't defer a function call if you need to check its error value"

Isn't the lesson here: If you must have a Close method that might fail in your API, ensure it can safely be called multiple times?

As long as that is true, you can approach it like you would any other API that has resources that might need to be cleaned up.

    f, _ := os.Create(...)
    defer f.Close()
    // perform writes and whatever else
    if err := f.Close(); err != nil {
        // recover from failure
    }

(os.File supports this, expectedly)

> the solution to this problem will be to use a WriteFile function

If it were the solution you'd already be using os.WriteFile. It has a time and place, but often it is not suitable. Notably because it requires the entire file contents to be first stored in memory, which can become problematic.

Certainly you could write a custom WriteFile function that is tuned to your specific requirements, but now you're back to needing to be familiar with the intricacies of a lower-level API in order to facilitate that.

Sure, that's an alternative, although it means there will be some code paths where the error returned by f.Close() becomes the error returned by the entire function and others where it is ignored (though you could easily log it). That might be fine, but you also might want to handle all the cases explicitly and return a combined error in a case where, say, a non-file-related operation fails and then the file also fails to close.

> becomes the error returned by the entire function

If you find the error returned by f.Close to be significant, are you sure returning again it is the right course of action? Most likely you want to do something more meaningful with that state, like retrying the write with an alternate storage device.

Returning the error is giving up, and giving up just because a file didn't close does not make for a very robust system. Not all programs need to be robust, necessarily, but Go is definitely geared towards building systems that are intended to be robust.

It obviously depends on the context. There's no general right or wrong answer to that question.

You seem confused. The article is about writing a file where it does matter, but the comment example, which is what we're talking about, only reads a file. If close fails after read, who gives a shit? What difference is it going to make? All your read operations are complete already. Close isn't going to trigger a time machine that goes back and time and undos the reads you've performed. It is entirely inconsequential.

Only if you can safely assume the OS, file system, or std lib cleans up any open file handles that failed to close; I'm 99% sure this is the case in 99% of cases, but there may be edge cases (very specific filesystems or hardware?) where it does matter? I don't know.

You can't safely assume that, but what are you going to do about it when it does fail? There is nothing you can do. There isn't usually a CloseClose function to use when Close fails. If Close fails, that's it. You're otherwise out of luck.

Certainly, in the write failure case where there is write failure you'd want to try writing to something else (ideally), notify someone that an operation didn't happen (as a last resort), or something to that effect in order to recover.

But in this case there is no need to try again and nobody really cares. Everything you needed the resources for is already successfully completed. If there is failure when releasing those resources, so what? There is nothing you can do about it.

> If close fails after read, who gives a shit?

ulimit -n

You ignore errors on close, and one morning you wake up with your app in CrashLoopBackoff with the final log message "too many files". How do you start debugging this?

Compare the process to the case where you do log errors, and your log is full of "close /mnt/some-terrible-fuse-filesystem/scratch.txt: input/output error". Still baffling of course, but you have some idea where to go next.

To start, you need to figure out why Kubernetes isn't retaining your stack trace/related metadata when the app crashes. That is the most pressing bug. Which is probably best left to the k9s team. You outsourced that aspect of the business of good reason, no doubt.

After they've fixed what they need to fix you need to use the information now being retained to narrow down why your app is crashing at all. Failing to open a file is expected behaviour. It should not be crashing.

Then maybe you can get around to looking at the close issue. But it's the least of your concerns. You've got way bigger problems to tackle first.

The app crashes because "too many files" includes the fd accept(2) wants to allocate so your app can respond to the health check.

A file not able to opened is expected, always! accept is no exception here. Your application should not be crashing because of it.

If I recall, Kubernetes performs health checks over HTTP, so presumably your application is using the standard library's http server to provide that? If so, accept is full abstracted away. So, if that's crashing, that's a bug in Go.

Is that for you to debug, or is it best passed on to the Go team?

There isn't a bug, it's resource exhaustion. You open a bunch of files and they fail to close. You don't log errors on the close, so you have no idea it's happening. Now your app is failing to open new file descriptors to accept HTTP connections. You get a fixed number of fds per app; ulimit -n. If you don't close files you've read, the descriptor is gone.

The bug in this case is in the filesystem that hangs on close. It happens on network filesystems. You can't return the fd to the kernel if your filesystem doesn't let you.

The bug of which we speak is in that your app is crashing. Exhausting open file handles is expected behaviour! Expected behaviour should not lead to a crash. Crashing is only for exceptional behaviour.

The filesystem hanging is unlikely to be a bug. The filesystems you'd realistically use in conjunction with Kubernetes are pretty heavily tested. More likely it is supposed to hang under whatever conditions has lead that to happen.

And, sure, maybe you'll eventually want to determine why the filesystem has moved into that failure state, but most pressing is that your app is crashing. All that work you put into gracefully handling the failing situation going to waste.

You're really hung up on Kubernetes but it was an incidental comment in a hypothetical story.

"You wake up and find out that Heroku's staff is anxiously awaiting your departure from your apartment to tell you that your app is down."

Kubernetes is really here nor there. It's the crashing of the app that is our focus. An app should not be crashing on expected behaviour.

That's clearly a bug, and the bug you need to fix first so that you can have your failsafes start working again. You asked where to start and that's the answer, unquestionably.

The app doesn't crash, it's deadlocked. It can't do any more work because to do future work it needs to accept TCP connections. It can't do that because it has hit a resource limit. It hit the resource limit because it didn't correctly close files. It can't close files because of a bug in the filesystem. You don't know this because you didn't log the errors.

I really don't know how I can make my explanation simpler.

> The app doesn't crash

You literally said that it crashes:

   The app crashes because "too many files" includes the fd accept(2) wants to allocate so your app can respond to the health check.

https://news.ycombinator.com/item?id=41505892

> I really don't know how I can make my explanation simpler.

Not making up some elaborate story that you now are trying to say didn't even happen would be a good start. What you are actually trying to communicate is not complicated at all. It didn't need a story. Not sure what you were thinking when you decided fiction writing was a good idea, but I certainly had fun making fun of you for it! So, at least it was not all for not.

>>>

      f, err := os.Open(blah)
      // ...
      defer CheckClose(f, &err)

What knowledge do you hope to gain of f.Close fails here?

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.

That's not it. You can't write to a file handle returned by os.Open.

https://pkg.go.dev/os#Open

    func Open(name string) (*File, error)
    Open opens the named file for reading. If successful, methods on the returned file can be used for reading; the associated file descriptor has mode O_RDONLY. If there is an error, it will be of type *PathError.

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!

I don't know how you can confidently say "if close fails, who cares" all over this thread.

If close fails, I wanna know and I wanna know why.

Let me introduce you to the question mark, seen on your screen as the '?' character. It indicates that a question is asked. This is important as...

Never has "who cares" been confidently said. It has always been asked "who cares?". And not asked in a vacuum either, but specifically asked alongside the question of what is to be gained from the knowledge of the error.

We now know that you allegedly care, which is a promising start. But you purposefully ignored the other question, which questions the credibly of your care. You can't meaningfully care about something if you don't know why you care about it, and if you knew you'd have told us about it already as it nonsensical to answer to the "who cares?" question alone, so...

I care about it because it's unexpected. I seek to wrap my mind as fully as possible around the behavior of my programs. Unexpected behavior like this indicates there is something I do not understand. I cannot rely on my programs if I do not understand them. I cannot state they are robust and stable if I do not understand these sorts of details.

Which of course isn't really a "solution", as it results in ambiguous error semantics; this is especially the case with Go's defer, as it pushes the code to the end of the entire function, not merely some intermediate relevant scope... the result is that a file used near the beginning of a function might fail to close but that error will not be realized until after something else in the function fails, after the point of no return on the close.

Really, this is kind of a fundamental limitation of automatic allocation semantics, which is effectively a monad that is being stacked with the error propagation monad in a confusing manner that means you "should" only defer operations which don't have errors.

Even in languages with exceptions, such as C++ and Java, they had to wrangle with this problem and failed to solve it: in C++ 11 or 17 or whatever, deconstructors are now by default nothrow in an attempt to prevent this kind of mistake.

...but then, what does one do with close?! In some sense, the entire concept of close must not fail, and yet it exists in a world where we don't really believe in anything that can't fail, as we like moving around failure semantics.

FWIW, Linus has suggested that the kernel should largely accept that application developers don't ever check the return value of close... but has also stated that developers should sync the file first if they care and also check close (at least, to be maximally correct).

But like, what does one do then? How do you ever recover from this? I actually think you can't, if you are in a cleanup operation... not without breaking your error regime. I think--and this is also where the article eventually goes in the updates (though without noting you can add your own boolean)--you should therefore both explicitly close (and/or maybe flush/sync) the file after writing to it and have a "if I didn't close this, close it" in your cleanup handler; critically, the (explicit) former checks for errors, while the (implicit) latter doesn't.

The solution is that there are multiple solutions that are suitable in different situations.

If you need "strict" requirements then it is likely impossible but you can get close enough if you use sqlite. Or more lightweight atomic/thread-safe/fault-tolerant file library. You could rollout your own: it easy to start, and continue until the error rate is tolerable for your application (though it may take more dev time).

If you don't need db-like strict guarantees. Just write your app knowing that it may fail (data may be lost, corrupted). It may be ok in a lot of cases.

> FWIW, Linus has suggested that the kernel should largely accept that application developers don't ever check the return value of close... but has also stated that developers should sync the file first if they care and also check close (at least, to be maximally correct).

Close just behaves very differently depending on the actual filesystem, too. Usually it’s very fast because it doesn’t do much of anything, but e.g. on NFS close will actually wait for writeback to the server to complete (due to close-to-open semantics).

I think some of those issues with errors could be worked around. Wrapping errors provides a way to return a "cleanup failed" error that also includes the root cause of being a failed write. Likewise, there are packages for handling multi-errors.

I think the reality is that most of us push anything where the return status of Close would be important into the database, specifically because it handles semantics like this and simultaneous writes for us. It's like half the selling point of SQLite; you could write JSON documents and handle all the edge cases yourself, or just jam it in SQLite and quite worrying about Close and simultaneous writes and all that junk.

I mean, we can cause the same kind of problem there, as people might try to write a scope exit handler to commit a transaction. You'd then run into the same issue, and so "commit transaction" isn't a thing which should ever be in such a construct. Of course, deleting the objects for the transaction / closing the database connection / etc. would be fine to ignore errors from (and hopefully wouldn't/shouldn't fail anyway) and so those can and should be automated: you defer close but manually call and check a commit.

Once you accept this reality, the file case is the same: putting the sync and/or first critical close inline is equivalent work. The issue is that you simply can't -- no matter what the mechanism is -- slip back and forth between your scope maintenance and your error handling monads, resulting in needing cleanup operations where failure is not an option; and, so, you either must not care about the error in the context of the call or must do something even more drastic like terminate the entire program for violating semantics.

FWIW, I do appreciate that people are less likely to make that kind of mistake when working with a database, as people largely get that you should even try to commit a transaction if the code in it had failed somehow. Additionally, I appreciate that if you have a very tight scope -- which Go makes hard, but can still be pulled off -- the "close and throw an error if and only if we don't have an error right now" strategy is not at all horrible... it just isn't a "solution" to the underlying issue without an understanding of why.

Put elsewise, I think it is useful to appreciate that there is more of a universal theoretical / math reason why this is awkward and why it kind of needs to be built in a specific way, and that this issue transcends the syntax or even the implementation details of how you are trying to manage errors: at the end of the end of the day, all of these techniques people discuss are in some sense equivalent, and, at best, most of these workarounds at offer are ways to incorrectly model the problem due to some systems giving you too much rope.

I don't think that holds for database transactions because of the semantics of rollbacks. Trying to do a rollback after committing is effectively a no-op (I believe it returns an error, but doesn't actually change the DB), so you can defer a rollback and only call commit on the happy path.

I don't believe people generally care about the error context on the rollback, which makes it safe to defer into a context that can't interact with the error handling monads. Rollbacks shouldn't generally fail, even if they do there's basically nothing you can do about it, and there are few differences between a successful and failed rollback beyond resources on the DB server until the connection is closed.

The Commit is the portion that contains the context people care about in their errors, and that is still safely in a context where it can interact with error handling.

I believe files can get similar atomicity, but it requires doing IO in strange ways. E.g. updating a file isn't atomic, but mv'ing one is. So you can copy the file you want to update into /tmp, update the copy, and then mv the copy to the original file (commit is mv'ing it, rollback is rm'ing it or just ignoring it).

Database transactions aren't atomic and do have the same issue if they reference external resources, though. E.g. if you have a database that stores an index of S3 files, transactions won't save you from writing a file to S3 but then failing to write a record for it into the database. That does muddle the error handling again.

I wouldn't mind the language adding some syntax for this, based on the java try-with-resource but using error values rather than exceptions. Something like

    f, err = with(os.Open(thing)) {
        // do stuff
    }
    // at this point err has the same semantics as your CheckClose

I believe your example doesn't introduce any new syntax though. Go is highly resistant to adding syntax (and that's a good thing, it keeps the tooling fast and barrier to entry low).

Reply on news.ycombinator.com